pbacsko commented on code in PR #1009:
URL: https://github.com/apache/yunikorn-core/pull/1009#discussion_r1926897947
##########
pkg/scheduler/partition.go:
##########
@@ -79,7 +79,29 @@ type PartitionContext struct {
locking.RWMutex
}
+// newPartitionContextForValidation initializes a shadow partition based on
the configuration.
+// The shadow partition is used to validate the configuration, it is not used
for scheduling.
+func newPartitionContextForValidation(conf configs.PartitionConfig, rmID
string, cc *ClusterContext) (*PartitionContext, error) {
Review Comment:
The return value `*PartitionContext` is never used from this function, so
you might as well just remove it and just call it `validateConfiguration()`.
Unless of course, you create unit tests which actually use it for some kind of
verification... See other comments.
##########
pkg/scheduler/partition.go:
##########
@@ -98,13 +120,41 @@ func newPartitionContext(conf configs.PartitionConfig,
rmID string, cc *ClusterC
foreignAllocs: make(map[string]*objects.Allocation),
}
pc.partitionManager = newPartitionManager(pc, cc)
- if err := pc.initialPartitionFromConfig(conf); err != nil {
- return nil, err
- }
return pc, nil
}
+// initialPartitionFromConfigForValidation is used to validate the partition
configuration.
+// It works similarly to initialPartitionFromConfig but neither logs the queue
creation, sends a queue event, logs the node sorting policy,
+// nor updates user settings.
+func (pc *PartitionContext) initialPartitionFromConfigForValidation(conf
configs.PartitionConfig) error {
+ if len(conf.Queues) == 0 || conf.Queues[0].Name != configs.RootQueue {
+ return fmt.Errorf("partition cannot be created without root
queue")
+ }
+
+ // Setup the queue structure: root first it should be the only queue at
this level
+ // Add the rest of the queue structure recursively
+ queueConf := conf.Queues[0]
+ var err error
+ if pc.root, err = objects.NewConfiguredQueueForValidation(queueConf,
nil); err != nil {
+ return err
+ }
+ // recursively add the queues to the root
+ if err = pc.addQueueForValidation(queueConf.Queues, pc.root); err !=
nil {
+ return err
+ }
+
+ // We need to pass in the locked version of the GetQueue function.
+ // Placing an application will not have a lock on the partition context.
+ pc.placementManager =
placement.NewPlacementManager(conf.PlacementRules, pc.GetQueue)
+ // get the user group cache for the partition
+ pc.userGroupCache = security.GetUserGroupCache("")
+ pc.updateNodeSortingPolicyForValidation(conf)
+ pc.updatePreemption(conf)
Review Comment:
These lines are unnecessary here. No return values are checked, so whether
they run or not is irrelevant.
##########
pkg/scheduler/partition.go:
##########
@@ -191,17 +252,27 @@ func (pc *PartitionContext) updatePartitionDetails(conf
configs.PartitionConfig)
return ugm.GetUserManager().UpdateConfig(queueConf, conf.Queues[0].Name)
}
+func (pc *PartitionContext) addQueueForValidation(conf []configs.QueueConfig,
parent *objects.Queue) error {
+ err := pc.addQueueInternal(conf, parent,
objects.NewConfiguredQueueForValidation)
+ return err
+}
+
// Process the config structure and create a queue info tree for this partition
func (pc *PartitionContext) addQueue(conf []configs.QueueConfig, parent
*objects.Queue) error {
+ err := pc.addQueueInternal(conf, parent, objects.NewConfiguredQueue)
+ return err
+}
+
+func (pc *PartitionContext) addQueueInternal(conf []configs.QueueConfig,
parent *objects.Queue, newQueueFn func(configs.QueueConfig, *objects.Queue)
(*objects.Queue, error)) error {
Review Comment:
Have you considered passing down a boolean flag `validate`? All these method
duplications - it's a bit dubious to me. For example, passing a function
pointer to run code whether it's a validation or not just doesn't seem right.
`NewConfiguredQueue()` definitely has some callers, but it's not the end of
the world. I don't know where others stand on this, but I'm in favor of a flag.
--
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]
