This is an automated email from the ASF dual-hosted git repository.
wilfred-s pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/yunikorn-core.git
The following commit(s) were added to refs/heads/master by this push:
new b8f126cd [YUNIKORN-3291] Remove redundant config passing in
mergeProperties (#1091)
b8f126cd is described below
commit b8f126cdfae6b8d48d8c2b05ed01179f13dae776
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]>
---
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]
