This is an automated email from the ASF dual-hosted git repository. wilfred-s pushed a commit to branch branch-1.9 in repository https://gitbox.apache.org/repos/asf/yunikorn-core.git
commit c8aafbdb27f2b87148de4364a21dbe2df2eb17a4 Author: Venkateshwaran Shanmugham <[email protected]> AuthorDate: Wed Jun 10 14:02:59 2026 +1000 [YUNIKORN-3291] Remove redundant config passing in mergeProperties (#1091) The config is already part of the queue object and does not need to be passed in again. Leverage the getProperties returned object to not create a new empty object to store the result in. Closes: #1091 Signed-off-by: Venkateshwaran Shanmugham <[email protected]> Signed-off-by: Wilfred Spiegelenburg <[email protected]> (cherry picked from commit b8f126cdfae6b8d48d8c2b05ed01179f13dae776) --- pkg/scheduler/objects/queue.go | 42 +++++++++++++++----------------- pkg/scheduler/objects/queue_test.go | 48 +++++++++++++++++++++++++++++-------- pkg/scheduler/partition.go | 7 +++--- 3 files changed, 60 insertions(+), 37 deletions(-) diff --git a/pkg/scheduler/objects/queue.go b/pkg/scheduler/objects/queue.go index 2fa9cca1..394a374f 100644 --- a/pkg/scheduler/objects/queue.go +++ b/pkg/scheduler/objects/queue.go @@ -146,8 +146,8 @@ func NewConfiguredQueue(conf configs.QueueConfig, parent *Queue, silence bool, a // add to the parent, we might have an overall lock already // still need to make sure we lock the parent so we do not interfere with scheduling if parent != nil { - // pull the properties from the parent that should be set on the child - sq.mergeProperties(parent.getProperties(), conf.Properties) + // inherit filtered parent properties; sq.properties already holds this queue's config from applyConf + sq.mergeProperties(parent.getProperties()) sq.UpdateQueueProperties(nil) err := parent.addChildQueue(sq) if err != nil { @@ -262,35 +262,31 @@ func (sq *Queue) GetProperties() map[string]string { return sq.getProperties() } -// MergeParentProperties merges the parent queue properties with config properties into this queue. -// Config properties override parent properties. This should be called after ApplyConf during -// config reload to re-apply inherited properties from the parent. -// Lock protected. -func (sq *Queue) MergeParentProperties(config map[string]string) { - // get parent properties outside of the lock to avoid potential deadlocks with parent queue lock. +// MergeParentProperties merges the parent queue properties with this queue's config properties. +// Config properties already set on the queue (from ApplyConf) override parent properties. +// This should be called after ApplyConf during config reload to re-apply inherited properties +// from the parent. Lock protected. +func (sq *Queue) MergeParentProperties() { + // get parent properties outside of the lock to avoid potential deadlocks with parent queue lock parentProps := sq.parent.getProperties() sq.Lock() defer sq.Unlock() - sq.mergeProperties(parentProps, config) + sq.mergeProperties(parentProps) } -// mergeProperties merges the properties from the parent queue and the config in the set from new queue +// mergeProperties merges filtered parent properties with this queue's config properties. +// Config properties already set on the queue (from applyConf) override parent properties. +// The parent map is a clean copy from getProperties() and becomes the resulting map. // lock free call -func (sq *Queue) mergeProperties(parent, config map[string]string) { - // clean out all existing values (handles update case) - sq.properties = make(map[string]string) - // Set the parent properties - if len(parent) != 0 { - for key, value := range parent { - sq.properties[key] = filterParentProperty(key, value) - } +func (sq *Queue) mergeProperties(parent map[string]string) { + config := sq.properties + for key, value := range parent { + parent[key] = filterParentProperty(key, value) } - // merge the config properties - if len(config) > 0 { - for key, value := range config { - sq.properties[key] = value - } + for key, value := range config { + parent[key] = value } + sq.properties = parent } func convertDelay(value string, def time.Duration) (time.Duration, error) { diff --git a/pkg/scheduler/objects/queue_test.go b/pkg/scheduler/objects/queue_test.go index ef453128..2f5ee3db 100644 --- a/pkg/scheduler/objects/queue_test.go +++ b/pkg/scheduler/objects/queue_test.go @@ -49,7 +49,9 @@ func TestMergeParentPropertiesNilParent(t *testing.T) { root, err := createRootQueue(nil) assert.NilError(t, err, "root queue create failed") root.properties = map[string]string{"key": "value"} - root.MergeParentProperties(map[string]string{"other": "other-value"}) + _, err = root.ApplyConf(configs.QueueConfig{Properties: map[string]string{"other": "other-value"}}) + assert.NilError(t, err, "applyConf failed") + root.MergeParentProperties() props := root.getProperties() _, exists := props["key"] @@ -67,7 +69,7 @@ func TestMergeParentPropertiesParentPropsInherited(t *testing.T) { child, err := createManagedQueue(parent, "leaf", false, nil) assert.NilError(t, err, "child queue create failed") - child.MergeParentProperties(nil) + child.MergeParentProperties() props := child.getProperties() assert.Equal(t, "inherited-value", props["inherited-key"], "child should inherit parent properties") @@ -81,7 +83,9 @@ func TestMergeParentPropertiesConfigOverridesParent(t *testing.T) { child, err := createManagedQueue(parent, "leaf", false, nil) assert.NilError(t, err, "child queue create failed") - child.MergeParentProperties(map[string]string{"key": "config-value", "extra": "extra-value"}) + _, err = child.ApplyConf(configs.QueueConfig{Properties: map[string]string{"key": "config-value", "extra": "extra-value"}}) + assert.NilError(t, err, "applyConf failed") + child.MergeParentProperties() props := child.getProperties() assert.Equal(t, "config-value", props["key"], "config property should override parent property") @@ -101,7 +105,9 @@ func TestMergeParentPropertiesClearsOldProperties(t *testing.T) { child.properties["stale-key"] = "stale-value" child.Unlock() - child.MergeParentProperties(map[string]string{"new-key": "new-value"}) + _, err = child.ApplyConf(configs.QueueConfig{Properties: map[string]string{"new-key": "new-value"}}) + assert.NilError(t, err, "applyConf failed") + child.MergeParentProperties() props := child.getProperties() _, exists := props["stale-key"] @@ -119,7 +125,7 @@ func TestMergeParentPropertiesFilterPriorityPolicy(t *testing.T) { child, err := createManagedQueue(parent, "leaf", false, nil) assert.NilError(t, err, "child queue create failed") - child.MergeParentProperties(nil) + child.MergeParentProperties() props := child.getProperties() // priority.policy should not be inherited from parent; filterParentProperty resets it to default @@ -137,7 +143,7 @@ func TestMergeParentPropertiesFilterPriorityOffset(t *testing.T) { child, err := createManagedQueue(parent, "leaf", false, nil) assert.NilError(t, err, "child queue create failed") - child.MergeParentProperties(nil) + child.MergeParentProperties() props := child.getProperties() // priority.offset should not be inherited; filterParentProperty resets it to "0" @@ -155,7 +161,7 @@ func TestMergeParentPropertiesFilterPreemptionPolicyDisabledPropagates(t *testin child, err := createManagedQueue(parent, "leaf", false, nil) assert.NilError(t, err, "child queue create failed") - child.MergeParentProperties(nil) + child.MergeParentProperties() props := child.getProperties() // disabled preemption.policy is the only value that propagates from parent @@ -173,7 +179,7 @@ func TestMergeParentPropertiesFilterPreemptionPolicyNonDisabledReset(t *testing. child, err := createManagedQueue(parent, "leaf", false, nil) assert.NilError(t, err, "child queue create failed") - child.MergeParentProperties(nil) + child.MergeParentProperties() props := child.getProperties() // non-disabled preemption.policy should not propagate from parent; reset to default @@ -193,10 +199,12 @@ func TestMergeParentPropertiesConfigCanOverrideFilteredProps(t *testing.T) { assert.NilError(t, err, "child queue create failed") // config explicitly sets these, so they should not be overridden by the parent filter - child.MergeParentProperties(map[string]string{ + _, err = child.ApplyConf(configs.QueueConfig{Properties: map[string]string{ configs.PriorityPolicy: policies.FencePriorityPolicy.String(), configs.PriorityOffset: "5", - }) + }}) + assert.NilError(t, err, "applyConf failed") + child.MergeParentProperties() props := child.getProperties() assert.Equal(t, policies.FencePriorityPolicy.String(), props[configs.PriorityPolicy], @@ -205,6 +213,26 @@ func TestMergeParentPropertiesConfigCanOverrideFilteredProps(t *testing.T) { "config priority.offset should override filtered parent value") } +func TestMergeParentPropertiesUsesApplyConfConfig(t *testing.T) { + root, err := createRootQueue(nil) + assert.NilError(t, err, "root queue create failed") + parent, err := createManagedQueueWithProps(root, "parent", true, nil, map[string]string{ + "inherited-key": "parent-value", + }) + assert.NilError(t, err, "parent queue create failed") + child, err := createManagedQueue(parent, "leaf", false, nil) + assert.NilError(t, err, "child queue create failed") + + // simulate config reload: ApplyConf sets only this queue's properties, then merge inherits from parent + _, err = child.ApplyConf(configs.QueueConfig{Properties: map[string]string{"own-key": "own-value"}}) + assert.NilError(t, err, "applyConf failed") + child.MergeParentProperties() + + props := child.getProperties() + assert.Equal(t, "parent-value", props["inherited-key"], "parent property should be inherited") + assert.Equal(t, "own-value", props["own-key"], "config property from ApplyConf should be present") +} + func TestQueueBasics(t *testing.T) { // create the root root, err := createRootQueue(nil) diff --git a/pkg/scheduler/partition.go b/pkg/scheduler/partition.go index 7daa94eb..b9489787 100644 --- a/pkg/scheduler/partition.go +++ b/pkg/scheduler/partition.go @@ -243,10 +243,9 @@ func (pc *PartitionContext) updateQueues(config []configs.QueueConfig, parent *o oldMax, err = queue.ApplyConf(queueConfig) if err == nil { // Re-apply inherited properties from parent, mirroring the NewConfiguredQueue path. - // ApplyConf sets sq.properties to only the queue's own config properties, which - // drops any previously inherited values and prevents parent property changes from - // propagating to existing child queues on config reload. - queue.MergeParentProperties(queueConfig.Properties) + // ApplyConf sets sq.properties to only this queue's config properties; merge uses + // those properties directly instead of passing config again. + queue.MergeParentProperties() } } if err != nil { --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
