wilfred-s commented on code in PR #1091:
URL: https://github.com/apache/yunikorn-core/pull/1091#discussion_r3377164776
##########
pkg/scheduler/objects/queue.go:
##########
@@ -262,21 +262,26 @@ 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.
- parentProps := sq.parent.getProperties()
+// 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() {
+ var parentProps map[string]string
+ if sq.parent != nil {
+ // get parent properties outside of the lock to avoid potential
deadlocks with parent queue lock
+ parentProps = sq.parent.getProperties()
+ }
Review Comment:
`getProperties()` is nil safe no need to test here or initialise the map,
that is all handled already.
##########
pkg/scheduler/objects/queue.go:
##########
@@ -262,21 +262,26 @@ 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.
- parentProps := sq.parent.getProperties()
+// 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() {
+ var parentProps map[string]string
+ if sq.parent != nil {
+ // 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.
// lock free call
-func (sq *Queue) mergeProperties(parent, config map[string]string) {
+func (sq *Queue) mergeProperties(parent map[string]string) {
+ config := sq.properties
// clean out all existing values (handles update case)
sq.properties = make(map[string]string)
Review Comment:
The map returned from `getProperties()` is a clean copy that we can use.
Store the results in the parent map. It will become the "resulting" map, no
new map needed etc.
##########
pkg/scheduler/objects/queue.go:
##########
Review Comment:
let range handle a nil or empty map
##########
pkg/scheduler/objects/queue.go:
##########
Review Comment:
store everything in the passed in `parent` map
replace `sq.properties` in a final action with `parent`
##########
pkg/scheduler/objects/queue.go:
##########
Review Comment:
let range handle the nil or empty map
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
