blueBlue0102 commented on code in PR #1009:
URL: https://github.com/apache/yunikorn-core/pull/1009#discussion_r1955643339
##########
pkg/scheduler/partition.go:
##########
@@ -79,7 +79,7 @@ type PartitionContext struct {
locking.RWMutex
}
-func newPartitionContext(conf configs.PartitionConfig, rmID string, cc
*ClusterContext) (*PartitionContext, error) {
+func newPartitionContext(conf configs.PartitionConfig, rmID string, cc
*ClusterContext, silence bool) (*PartitionContext, error) {
if conf.Name == "" || rmID == "" {
log.Log(log.SchedPartition).Info("partition cannot be created",
Review Comment:
If we still want to use the silence flag approach, we should check silence
before logging.
##########
pkg/scheduler/context.go:
##########
@@ -365,7 +365,7 @@ func (cc *ClusterContext) updateSchedulerConfig(conf
*configs.SchedulerConfig, r
part, ok := cc.partitions[p.Name]
if ok {
// make sure the new info passes all checks
- _, err = newPartitionContext(p, rmID, nil)
+ _, err = newPartitionContext(p, rmID, nil, true)
if err != nil {
Review Comment:
It seems we call `newPartitionContext` here only for validating
`configs.PartitionConfig`.
If my understanding is correct, the main purpose of this JIRA is to avoid
any side effects when validating the config before `updatePartitionDetails`,
**not only**:
- Logging
- Sending out events
**but also including**:
- Loading the user manager and updating user settings.
`ugm.GetUserManager().UpdateConfig()` would be called in
`updatePartitionDetails()` when validation passes.
- etc.
So rather than adding a lot of flag parameters to each underlying function,
extracting all the validation logic from `newPartitionContext` into another
function or adding those validation logic into `updatePartitionDetails` might
be a better approach.
Let `newPartitionContext` do what it needs to do without consideration of
"silence". What do you think?
##########
pkg/common/security/acl.go:
##########
@@ -116,7 +119,7 @@ func NewACL(aclStr string) (ACL, error) {
// trim and check for wildcard
acl.setAllAllowed(aclStr)
// parse users and groups
- acl.setUsers(strings.Split(fields[0], common.Separator))
+ acl.setUsers(strings.Split(fields[0], common.Separator), silence)
if len(fields) == 2 {
acl.setGroups(strings.Split(fields[1], common.Separator))
Review Comment:
If we still want to use the silence flag approach, `acl.setGroups` also
needs to be silenced.
--
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]
