-
Notifications
You must be signed in to change notification settings - Fork 217
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Task Framework IdealState Removal #1326
Task Framework IdealState Removal #1326
Conversation
helix-core/src/main/java/org/apache/helix/controller/rebalancer/util/RebalanceScheduler.java
Outdated
Show resolved
Hide resolved
helix-core/src/main/java/org/apache/helix/task/assigner/ThreadCountBasedTaskAssigner.java
Show resolved
Hide resolved
.../test/java/org/apache/helix/controller/stages/TestQuotaConstraintSkipWorkflowAssignment.java
Show resolved
Hide resolved
helix-core/src/test/java/org/apache/helix/task/TestGetLastScheduledTaskExecInfo.java
Show resolved
Hide resolved
helix-core/src/main/java/org/apache/helix/controller/stages/task/TaskSchedulingStage.java
Show resolved
Hide resolved
helix-core/src/main/java/org/apache/helix/task/AbstractTaskDispatcher.java
Show resolved
Hide resolved
helix-core/src/main/java/org/apache/helix/controller/stages/ResourceComputationStage.java
Outdated
Show resolved
Hide resolved
@NealSun96 Thanks for your PR. I think the PR is in good shape now. Can you please add an integration test for resource to drop? That can conclude this PR. In the meanwhile, I will review the code again. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM. Please run mvn test multiple times just to make sure everything is in order.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm still reviewing the code, but here is one concerning point. Please double check. Thanks.
helix-core/src/main/java/org/apache/helix/controller/stages/ResourceComputationStage.java
Outdated
Show resolved
Hide resolved
helix-core/src/main/java/org/apache/helix/controller/stages/ResourceComputationStage.java
Outdated
Show resolved
Hide resolved
helix-core/src/main/java/org/apache/helix/controller/stages/ResourceComputationStage.java
Outdated
Show resolved
Hide resolved
helix-core/src/main/java/org/apache/helix/controller/stages/ResourceComputationStage.java
Show resolved
Hide resolved
helix-core/src/main/java/org/apache/helix/controller/stages/ResourceComputationStage.java
Outdated
Show resolved
Hide resolved
helix-core/src/main/java/org/apache/helix/controller/stages/ResourceComputationStage.java
Outdated
Show resolved
Hide resolved
helix-core/src/main/java/org/apache/helix/controller/stages/ResourceComputationStage.java
Outdated
Show resolved
Hide resolved
helix-core/src/main/java/org/apache/helix/controller/stages/ResourceComputationStage.java
Outdated
Show resolved
Hide resolved
helix-core/src/main/java/org/apache/helix/controller/stages/ResourceComputationStage.java
Outdated
Show resolved
Hide resolved
helix-core/src/main/java/org/apache/helix/controller/stages/ResourceComputationStage.java
Show resolved
Hide resolved
helix-core/src/main/java/org/apache/helix/controller/stages/AttributeName.java
Outdated
Show resolved
Hide resolved
helix-core/src/main/java/org/apache/helix/controller/stages/ResourceComputationStage.java
Show resolved
Hide resolved
helix-core/src/main/java/org/apache/helix/controller/stages/ResourceComputationStage.java
Show resolved
Hide resolved
helix-core/src/main/java/org/apache/helix/controller/stages/ResourceComputationStage.java
Outdated
Show resolved
Hide resolved
helix-core/src/main/java/org/apache/helix/controller/stages/ResourceComputationStage.java
Show resolved
Hide resolved
helix-core/src/main/java/org/apache/helix/controller/stages/task/TaskSchedulingStage.java
Outdated
Show resolved
Hide resolved
helix-core/src/main/java/org/apache/helix/task/AbstractTaskDispatcher.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good in general. Please address the remaining comments.
BTW, please remove helix-core/test-results.txt : )
helix-core/src/main/java/org/apache/helix/controller/stages/ResourceComputationStage.java
Show resolved
Hide resolved
helix-core/src/main/java/org/apache/helix/controller/stages/ResourceComputationStage.java
Outdated
Show resolved
Hide resolved
helix-core/src/main/java/org/apache/helix/controller/stages/ResourceComputationStage.java
Show resolved
Hide resolved
helix-core/src/main/java/org/apache/helix/task/AbstractTaskDispatcher.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code looks good to me.
But do you want to keep some test logic to verify that IS does NOT exist anymore even the jobs are running normally? Or do we already have a test case about it?
After an offline discussion, we came to the conclusion that writing a test for feature removal is awkward and tricky to do right, and therefore it's not worth the effort at this moment. |
This PR is ready to be merged, approved by @jiajunwang Remove IdealState Dependency from Task FrameworkThis PR removes IdealState usage from Task Framework. The participant-side no longer creates IdealState when workflows/jobs are created. The controller-side no longer reads IdealState to create resources for Task Framework. |
Issues
Fixes #1323, #1324
Description
This PR removes IdealState usage from the task framework pipeline. Instead, now workflow resources are created directly using WorkflowConfig and JobConfig. As a result, the legacy pipeline logic in TaskSchedulingStage is removed: it was there for the case of "IdealState exists but not WorkflowConfig", which is no longer possible now.
After the removal of legacy pipeline logic, numerous unintended usage of the legacy pipeline were uncovered in the form of broken tests; these tests are addressed and fixed.
At the same time, 2 bugs in the pipeline were also uncovered and fixed: the first one is about tasks being incorrectly rejected; the second one is about negative scheduling delay not properly rejected.
Tests
Rerun
Documentation (Optional)
https://github.com/apache/helix/wiki/Task-Framework-IdealState-Dependency-Removal-Progression
Commits
Code Quality
(helix-style-intellij.xml if IntelliJ IDE is used)