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 2021/01/07 02:33:27 UTC
[GitHub] [incubator-yunikorn-core] TaoYang526 opened a new pull request #237: [Draft] Make applications and requests sorting process pluggable
TaoYang526 opened a new pull request #237:
URL: https://github.com/apache/incubator-yunikorn-core/pull/237
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-core] TaoYang526 commented on pull request #237: [Draft] Make applications and requests sorting process pluggable
Posted by GitBox <gi...@apache.org>.
TaoYang526 commented on pull request #237:
URL: https://github.com/apache/incubator-yunikorn-core/pull/237#issuecomment-758436680
Thanks @yangwwei and @wilfred-s for the review.
That makes sense to me, perhaps we can move plugins into `scheduler/policies` package and rename XXXPlugin to XXXPolicyPlugin which means the policy plugin of managing and scheduling for applications/requests/nodes(in future) ?
Another thing is that I want to update Requests#AddRequest to Requests#AddOrUpdateRequest and that should also be called when updating the pending repeat, so that the Requests instance can be notified in time which can be useful in a custom RequestsPlugin. Is that ok for you?
Hope to hear your thoughts about these.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-core] yangwwei commented on pull request #237: [Draft] Make applications and requests sorting process pluggable
Posted by GitBox <gi...@apache.org>.
yangwwei commented on pull request #237:
URL: https://github.com/apache/incubator-yunikorn-core/pull/237#issuecomment-756606521
hi @TaoYang526 thanks for the draft PR. At the first glance, I wasn't able to fully understand some of the places. After reading more info from the previous design doc in https://issues.apache.org/jira/browse/YUNIKORN-1. They start to make sense. I pretty like interfaces defined in this PR, the confusion part is queue_request_manager_plugin.go. I feel we can somehow optimize this to make it simpler. I will look into more details and get back to you. Thanks.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-core] yangwwei commented on pull request #237: [Draft] Make applications and requests sorting process pluggable
Posted by GitBox <gi...@apache.org>.
yangwwei commented on pull request #237:
URL: https://github.com/apache/incubator-yunikorn-core/pull/237#issuecomment-757425069
hi @TaoYang526 the structure now looks much better. Overall it looks good, some high-level changes might be needed:
1. can we remove interface `QueueRequestManager`? I think it is no longer needed
2. For interface `Application`, I think we can further simplify the interface by removing the following methods such as `CurrentState()`, `IsStarting()`, `IsAccepted()`, `IsNew()`.. etc. I understand now there are needed before the App state is an internal struct in cache package, we need to think about move them to the common package, or SI package.
3. Nit about the package naming. `plugins/defaults/` looks like a better name for default implementations for apps/requests.
@wilfred-s pls take a look at the latest changes and let us know your opinion about this. Thanks
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-core] TaoYang526 commented on pull request #237: [Draft] Make applications and requests sorting process pluggable
Posted by GitBox <gi...@apache.org>.
TaoYang526 commented on pull request #237:
URL: https://github.com/apache/incubator-yunikorn-core/pull/237#issuecomment-757272356
Thanks @yangwwei for your suggestion.
I agree that queue_request_manager_plugin is not easy to be understood, so I have refactored that plugin in latest commit: add applications plugin and let applications/requests plugins have their own sorting methods.
I hope now it can be simple as expected and please feel free to share your thoughts. Thanks.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-core] TaoYang526 commented on pull request #237: [Draft] Make applications and requests sorting process pluggable
Posted by GitBox <gi...@apache.org>.
TaoYang526 commented on pull request #237:
URL: https://github.com/apache/incubator-yunikorn-core/pull/237#issuecomment-757671014
Thanks @yangwwei for the review.
I have addressed your comments in the latest commit, only one point need to be discussed is that we should check the state of an application when the sort policy is `StateAwarePolicy`, so I still leave `CurrentState()` in `application` interface but removed other check methods like `IsStarting()` etc, and check the state via string comparison in `pkg/plugins/defaults/sorters.go#StateAwareFilter`. Is that ok?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-core] TaoYang526 commented on pull request #237: [Draft] Make applications and requests sorting process pluggable
Posted by GitBox <gi...@apache.org>.
TaoYang526 commented on pull request #237:
URL: https://github.com/apache/incubator-yunikorn-core/pull/237#issuecomment-757272356
Thanks @yangwwei for your suggestion.
I agree that queue_request_manager_plugin is not easy to be understood, so I have refactored that plugin in latest commit: add applications plugin and let applications/requests plugins have their own sorting methods.
I hope now it can be simple as expected and please feel free to share your thoughts. Thanks.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-core] yangwwei commented on pull request #237: [Draft] Make applications and requests sorting process pluggable
Posted by GitBox <gi...@apache.org>.
yangwwei commented on pull request #237:
URL: https://github.com/apache/incubator-yunikorn-core/pull/237#issuecomment-758385641
I had a sync up with @wilfred-s about this, I think in general the interfaces are looking good.
He had some concern about how plugins got initialized, he thinks the plugin package currently is mostly used by core-shim communication that may be only used by the K8shim. He suggested looking at the direction of moving the init logic to a separate package, just for scheduling policies (which is a pure core side thing). @TaoYang526 , what's your thought about this?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-core] TaoYang526 commented on pull request #237: [Draft] Make applications and requests sorting process pluggable
Posted by GitBox <gi...@apache.org>.
TaoYang526 commented on pull request #237:
URL: https://github.com/apache/incubator-yunikorn-core/pull/237#issuecomment-765052820
@yangwwei I have moved scheduling plugins which are only used in core scheduling process into a dependent package (scheduler/plugins) and make it configurable in the latest commit. For now there is no entry-point for updating those plugins, but we can easily call `plugins#Init(registry, pluginsConfig)` to refresh those plugins when plugins can be configured in future. Please help to review it again. Thanks.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [incubator-yunikorn-core] TaoYang526 commented on pull request #237: [Draft] Make applications and requests sorting process pluggable
Posted by GitBox <gi...@apache.org>.
TaoYang526 commented on pull request #237:
URL: https://github.com/apache/incubator-yunikorn-core/pull/237#issuecomment-765052820
@yangwwei I have moved scheduling plugins which are only used in core scheduling process into a dependent package (scheduler/plugins) and make it configurable in the latest commit. For now there is no entry-point for updating those plugins, but we can easily call `plugins#Init(registry, pluginsConfig)` to refresh those plugins when plugins can be configured in future. Please help to review it again. Thanks.
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org