You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@yunikorn.apache.org by GitBox <gi...@apache.org> on 2022/12/13 12:56:01 UTC
[GitHub] [yunikorn-core] pbacsko opened a new pull request, #476: [YUNIKORN-1464] Add new queue configuration options
pbacsko opened a new pull request, #476:
URL: https://github.com/apache/yunikorn-core/pull/476
### What is this PR for?
Add new queue properties "preemption.policy" and "preemption.delay"
### What type of PR is it?
* [ ] - Bug Fix
* [ ] - Improvement
* [x] - Feature
* [ ] - Documentation
* [ ] - Hot Fix
* [ ] - Refactoring
### Todos
* [ ] - Task
### What is the Jira issue?
https://issues.apache.org/jira/browse/YUNIKORN-1464
### How should this be tested?
### Screenshots (if appropriate)
### Questions:
* [ ] - The licenses files need update.
* [ ] - There is breaking changes for older versions.
* [ ] - It needs documentation.
--
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: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] craigcondit commented on a diff in pull request #476: [YUNIKORN-1464] Add new queue configuration options
Posted by GitBox <gi...@apache.org>.
craigcondit commented on code in PR #476:
URL: https://github.com/apache/yunikorn-core/pull/476#discussion_r1047323691
##########
pkg/scheduler/objects/queue.go:
##########
@@ -296,39 +297,64 @@ func (sq *Queue) setTemplate(conf configs.ChildTemplate) error {
return nil
}
-// UpdateSortType updates the sortType for the queue based on the current properties.
-func (sq *Queue) UpdateSortType() {
+// UpdateQueueProperties updates the queue properties defined as text
+func (sq *Queue) UpdateQueueProperties() {
sq.Lock()
defer sq.Unlock()
// set the defaults, override with what is in the configured properties
if sq.isLeaf {
// walk over all properties and process
var err error
- sq.sortType = policies.Undefined
for key, value := range sq.properties {
- if key == configs.ApplicationSortPolicy {
+ switch key {
+ case configs.ApplicationSortPolicy:
sq.sortType, err = policies.SortPolicyFromString(value)
if err != nil {
log.Logger().Debug("application sort property configuration error",
zap.Error(err))
}
- } else {
+ // if it is not defined default to fifo
+ if sq.sortType == policies.Undefined {
+ sq.sortType = policies.FifoSortPolicy
+ }
+ case configs.PreemptionPolicy:
+ sq.preemptionFenced, err = isFencingEnabled(value)
+ if err != nil {
+ log.Logger().Debug("queue fencing property configuration error",
+ zap.Error(err))
+ }
+ case configs.PreemptionDelay:
+ sq.preemptionDelay, err = time.ParseDuration(value)
+ if err != nil {
+ log.Logger().Debug("preemption delay property configuration error",
+ zap.Error(err))
+ }
+ default:
// skip unknown properties just log them
log.Logger().Debug("queue property skipped",
zap.String("key", key),
zap.String("value", value))
}
}
- // if it is not defined default to fifo
- if sq.sortType == policies.Undefined {
- sq.sortType = policies.FifoSortPolicy
- }
+
return
}
// set the sorting type for parent queues
sq.sortType = policies.FairSortPolicy
}
+func isFencingEnabled(str string) (bool, error) {
+ switch str {
+ // fifo is the default policy when not set
+ case "default":
+ return false, nil
+ case "fence":
+ return true, nil
+ default:
Review Comment:
We need to also add "disabled" to this as that's going to be how we disable preemption for a queue. Also, we should probably have these be constants.
##########
pkg/scheduler/objects/queue.go:
##########
@@ -296,39 +297,64 @@ func (sq *Queue) setTemplate(conf configs.ChildTemplate) error {
return nil
}
-// UpdateSortType updates the sortType for the queue based on the current properties.
-func (sq *Queue) UpdateSortType() {
+// UpdateQueueProperties updates the queue properties defined as text
+func (sq *Queue) UpdateQueueProperties() {
sq.Lock()
defer sq.Unlock()
// set the defaults, override with what is in the configured properties
if sq.isLeaf {
// walk over all properties and process
var err error
- sq.sortType = policies.Undefined
for key, value := range sq.properties {
- if key == configs.ApplicationSortPolicy {
+ switch key {
+ case configs.ApplicationSortPolicy:
sq.sortType, err = policies.SortPolicyFromString(value)
if err != nil {
log.Logger().Debug("application sort property configuration error",
zap.Error(err))
}
- } else {
+ // if it is not defined default to fifo
+ if sq.sortType == policies.Undefined {
+ sq.sortType = policies.FifoSortPolicy
+ }
+ case configs.PreemptionPolicy:
+ sq.preemptionFenced, err = isFencingEnabled(value)
+ if err != nil {
+ log.Logger().Debug("queue fencing property configuration error",
+ zap.Error(err))
+ }
+ case configs.PreemptionDelay:
+ sq.preemptionDelay, err = time.ParseDuration(value)
+ if err != nil {
+ log.Logger().Debug("preemption delay property configuration error",
+ zap.Error(err))
+ }
Review Comment:
We need to ensure that preemptionDelay is positive, and default to 30s if not specified.
--
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: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] craigcondit closed pull request #476: [YUNIKORN-1464] Add new queue configuration options
Posted by GitBox <gi...@apache.org>.
craigcondit closed pull request #476: [YUNIKORN-1464] Add new queue configuration options
URL: https://github.com/apache/yunikorn-core/pull/476
--
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: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [yunikorn-core] craigcondit commented on a diff in pull request #476: [YUNIKORN-1464] Add new queue configuration options
Posted by GitBox <gi...@apache.org>.
craigcondit commented on code in PR #476:
URL: https://github.com/apache/yunikorn-core/pull/476#discussion_r1047577283
##########
pkg/scheduler/objects/queue.go:
##########
@@ -296,39 +297,64 @@ func (sq *Queue) setTemplate(conf configs.ChildTemplate) error {
return nil
}
-// UpdateSortType updates the sortType for the queue based on the current properties.
-func (sq *Queue) UpdateSortType() {
+// UpdateQueueProperties updates the queue properties defined as text
+func (sq *Queue) UpdateQueueProperties() {
sq.Lock()
defer sq.Unlock()
// set the defaults, override with what is in the configured properties
if sq.isLeaf {
// walk over all properties and process
var err error
- sq.sortType = policies.Undefined
for key, value := range sq.properties {
- if key == configs.ApplicationSortPolicy {
+ switch key {
+ case configs.ApplicationSortPolicy:
sq.sortType, err = policies.SortPolicyFromString(value)
if err != nil {
log.Logger().Debug("application sort property configuration error",
zap.Error(err))
}
- } else {
+ // if it is not defined default to fifo
+ if sq.sortType == policies.Undefined {
+ sq.sortType = policies.FifoSortPolicy
+ }
+ case configs.PreemptionPolicy:
+ sq.preemptionFenced, err = isFencingEnabled(value)
+ if err != nil {
+ log.Logger().Debug("queue fencing property configuration error",
+ zap.Error(err))
+ }
+ case configs.PreemptionDelay:
+ sq.preemptionDelay, err = time.ParseDuration(value)
+ if err != nil {
+ log.Logger().Debug("preemption delay property configuration error",
+ zap.Error(err))
+ }
Review Comment:
Also, preemption policy needs to be supported on parent queues as well, so we can't just apply these in leaf queues.
--
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: reviews-unsubscribe@yunikorn.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org