Make applications and requests sorting process pluggable

[TaoYang526]

Discussion: https://issues.apache.org/jira/browse/YUNIKORN-1?focusedCommentId=17260177&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-17260177

[yangwwei]

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.

[TaoYang526]

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.

[yangwwei]

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

[TaoYang526]

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?

[yangwwei]

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?

[TaoYang526]

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.

[TaoYang526]

@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.

[wilfred-s]

This PR has been in the same state for 2 years. Lots has changed in the YuniKorn code. If this is still something that needs to be implemented please open a jira to go through the design.