You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@helix.apache.org by GitBox <gi...@apache.org> on 2021/01/06 05:22:55 UTC
[GitHub] [helix] alirezazamani opened a new pull request #1600: Eliminate redundant workflow context writes
alirezazamani opened a new pull request #1600:
URL: https://github.com/apache/helix/pull/1600
### Issues
- [X] My PR addresses the following Helix issues and references them in the PR description:
Fixes #1599
### Description
- [x] Here are some details about my PR, including screenshots of any UI changes:
In this PR, the workflow context will only be written to ZK if it has been modified. Otherwise, there is no need to update the context information in the ZK.
### Tests
- [x] The following tests are written for this issue:
TestContextRedundantUpdates
More test will be added to this file as we move toward reducing context writes
- [x] The following is the result of the "mvn test" command on the appropriate module:
Helix-core:
```
[INFO] Results:
[INFO]
[ERROR] Failures:
[ERROR] org.apache.helix.integration.rebalancer.CrushRebalancers.TestCrushAutoRebalanceNonRack.testLackEnoughInstances(org.apache.helix.integration.rebalancer.CrushRebalancers.TestCrushAutoRebalanceNonRack)
[ERROR] Run 1: TestCrushAutoRebalanceNonRack.testLackEnoughInstances:281 » Helix Failed to dr...
[ERROR] Run 2: TestCrushAutoRebalanceNonRack.testLackEnoughInstances:273 » Helix Cluster CLUS...
[INFO]
[INFO]
[ERROR] Tests run: 1255, Failures: 1, Errors: 0, Skipped: 0
[INFO]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD FAILURE
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 01:31 h
[INFO] Finished at: 2021-01-05T18:07:57-08:00
[INFO] ------------------------------------------------------------------------
```
The failed test passed when I run it individually.
mvn test -Dtest="TestCrushAutoRebalanceNonRack"
```
[INFO] Results:
[INFO]
[INFO] Tests run: 8, Failures: 0, Errors: 0, Skipped: 0
[INFO]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 36.248 s
[INFO] Finished at: 2021-01-05T21:17:53-08:00
[INFO] ------------------------------------------------------------------------
```
Helix-rest:
```
[INFO] Tests run: 171, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 101.012 s - in TestSuite
[INFO]
[INFO] Results:
[INFO]
[INFO] Tests run: 171, Failures: 0, Errors: 0, Skipped: 0
[INFO]
[INFO] ------------------------------------------------------------------------
[INFO] BUILD SUCCESS
[INFO] ------------------------------------------------------------------------
[INFO] Total time: 01:46 min
[INFO] Finished at: 2021-01-05T21:20:48-08:00
[INFO] ------------------------------------------------------------------------
```
### Commits
- My commits all reference appropriate Apache Helix GitHub issues in their subject lines. In addition, my commits follow the guidelines from "[How to write a good git commit message](http://chris.beams.io/posts/git-commit/)":
1. Subject is separated from body by a blank line
1. Subject is limited to 50 characters (not including Jira issue reference)
1. Subject does not end with a period
1. Subject uses the imperative mood ("add", not "adding")
1. Body wraps at 72 characters
1. Body explains "what" and "why", not "how"
### Code Quality
- My diff has been formatted using helix-style.xml
(helix-style-intellij.xml if IntelliJ IDE is used)
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] alirezazamani commented on a change in pull request #1600: Eliminate redundant workflow context writes
Posted by GitBox <gi...@apache.org>.
alirezazamani commented on a change in pull request #1600:
URL: https://github.com/apache/helix/pull/1600#discussion_r553528626
##########
File path: helix-core/src/main/java/org/apache/helix/task/WorkflowContext.java
##########
@@ -81,15 +85,20 @@ public void setJobState(String job, TaskState s) {
if (states == null) {
states = new TreeMap<String, String>();
_record.setMapField(WorkflowContextProperties.JOB_STATES.name(), states);
+ markWorkflowContextAsModified();
Review comment:
Done.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] dasahcc commented on a change in pull request #1600: Eliminate redundant workflow context writes
Posted by GitBox <gi...@apache.org>.
dasahcc commented on a change in pull request #1600:
URL: https://github.com/apache/helix/pull/1600#discussion_r553521658
##########
File path: helix-core/src/main/java/org/apache/helix/task/WorkflowContext.java
##########
@@ -200,23 +222,37 @@ public long getFinishTime() {
}
public void setLastScheduledSingleWorkflow(String workflow) {
- _record.setSimpleField(WorkflowContextProperties.LAST_SCHEDULED_WORKFLOW.name(), workflow);
+ String lastScheduleWorkflow =
+ _record.getSimpleField(WorkflowContextProperties.LAST_SCHEDULED_WORKFLOW.name());
+ if (!workflow.equals(lastScheduleWorkflow)) {
+ _record.setSimpleField(WorkflowContextProperties.LAST_SCHEDULED_WORKFLOW.name(), workflow);
+ markWorkflowContextAsModified();
+ }
// Record scheduled workflow into the history list as well
- List<String> workflows = getScheduledWorkflows();
- if (workflows == null) {
- workflows = new ArrayList<String>();
- _record.setListField(WorkflowContextProperties.SCHEDULED_WORKFLOWS.name(), workflows);
+ List<String> scheduledWorkflows = getScheduledWorkflows();
+ if (scheduledWorkflows == null) {
+ scheduledWorkflows = new ArrayList<String>();
+ _record.setListField(WorkflowContextProperties.SCHEDULED_WORKFLOWS.name(),
+ scheduledWorkflows);
+ markWorkflowContextAsModified();
+ }
+ if (!scheduledWorkflows.contains(workflow)) {
Review comment:
Same here. Either you add the workflows in previous block. Or remove markWorkflowContextAsModified() in previous one.
##########
File path: helix-core/src/main/java/org/apache/helix/task/WorkflowContext.java
##########
@@ -81,15 +85,20 @@ public void setJobState(String job, TaskState s) {
if (states == null) {
states = new TreeMap<String, String>();
_record.setMapField(WorkflowContextProperties.JOB_STATES.name(), states);
+ markWorkflowContextAsModified();
+ }
+ if (!s.name().equals(states.get(job))) {
Review comment:
states.get(job) could be null, right? If the previous block happens.
##########
File path: helix-core/src/main/java/org/apache/helix/task/WorkflowContext.java
##########
@@ -81,15 +85,20 @@ public void setJobState(String job, TaskState s) {
if (states == null) {
states = new TreeMap<String, String>();
_record.setMapField(WorkflowContextProperties.JOB_STATES.name(), states);
+ markWorkflowContextAsModified();
Review comment:
This is not necessary. If the block has been entered, it definitely will go through next block.
##########
File path: helix-core/src/main/java/org/apache/helix/task/WorkflowContext.java
##########
@@ -48,22 +48,26 @@
LAST_PURGE_TIME,
StartTime, // TODO this should be named JOB_SCHEDULED_START_TIME, it's not the actual start time of the job
NAME
- }
+ }
public static final int NOT_STARTED = -1;
public static final int UNFINISHED = -1;
+ // Note: This field needs to be set if any of the workflow context fields have been changed.
+ // Otherwise, the context will not be written to ZK by the controller.
+ private boolean isModified;
+
public WorkflowContext(ZNRecord record) {
super(record);
+ isModified = false;
}
public void setWorkflowState(TaskState s) {
String workflowState = _record.getSimpleField(WorkflowContextProperties.STATE.name());
- if (workflowState == null) {
- _record.setSimpleField(WorkflowContextProperties.STATE.name(), s.name());
- } else if (!workflowState.equals(TaskState.FAILED.name()) && !workflowState
- .equals(TaskState.COMPLETED.name())) {
+ if (workflowState == null || (!workflowState.equals(TaskState.FAILED.name())
Review comment:
We can move the definition of terminal states from Workflow/Job Dispatcher to some constant file. Then let's check whether the state whether in the terminal state.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] NealSun96 commented on a change in pull request #1600: Eliminate redundant workflow context writes
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #1600:
URL: https://github.com/apache/helix/pull/1600#discussion_r553511478
##########
File path: helix-core/src/main/java/org/apache/helix/task/WorkflowContext.java
##########
@@ -48,22 +48,26 @@
LAST_PURGE_TIME,
StartTime, // TODO this should be named JOB_SCHEDULED_START_TIME, it's not the actual start time of the job
NAME
- }
+ }
public static final int NOT_STARTED = -1;
public static final int UNFINISHED = -1;
+ // Note: This field needs to be set if any of the workflow context fields have been changed.
+ // Otherwise, the context will not be written to ZK by the controller.
+ private boolean isModified;
+
public WorkflowContext(ZNRecord record) {
super(record);
+ isModified = false;
}
public void setWorkflowState(TaskState s) {
String workflowState = _record.getSimpleField(WorkflowContextProperties.STATE.name());
- if (workflowState == null) {
- _record.setSimpleField(WorkflowContextProperties.STATE.name(), s.name());
- } else if (!workflowState.equals(TaskState.FAILED.name()) && !workflowState
- .equals(TaskState.COMPLETED.name())) {
+ if (workflowState == null || (!workflowState.equals(TaskState.FAILED.name())
Review comment:
This one could also be consolidated the same way, cutting out `workflowState == null`.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] kaisun2000 commented on pull request #1600: Eliminate redundant workflow context writes
Posted by GitBox <gi...@apache.org>.
kaisun2000 commented on pull request #1600:
URL: https://github.com/apache/helix/pull/1600#issuecomment-755698925
We should use the result of github run as standard now. Does it pass now?
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] alirezazamani commented on pull request #1600: Eliminate redundant workflow context writes
Posted by GitBox <gi...@apache.org>.
alirezazamani commented on pull request #1600:
URL: https://github.com/apache/helix/pull/1600#issuecomment-756455605
This PR is ready to be merged.
Eliminate redundant workflow context writes
In this commit, the workflow context will only be written to ZK
if it has been modified. Otherwise, there is no need to update the
context information in the ZK.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] alirezazamani commented on a change in pull request #1600: Eliminate redundant workflow context writes
Posted by GitBox <gi...@apache.org>.
alirezazamani commented on a change in pull request #1600:
URL: https://github.com/apache/helix/pull/1600#discussion_r553523548
##########
File path: helix-core/src/main/java/org/apache/helix/task/WorkflowContext.java
##########
@@ -81,15 +85,20 @@ public void setJobState(String job, TaskState s) {
if (states == null) {
states = new TreeMap<String, String>();
_record.setMapField(WorkflowContextProperties.JOB_STATES.name(), states);
+ markWorkflowContextAsModified();
+ }
+ if (!s.name().equals(states.get(job))) {
Review comment:
Yes, it could happen. But since (!noNull.equal(null)) will be true, then we will go to the if statement. Right?
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] alirezazamani commented on a change in pull request #1600: Eliminate redundant workflow context writes
Posted by GitBox <gi...@apache.org>.
alirezazamani commented on a change in pull request #1600:
URL: https://github.com/apache/helix/pull/1600#discussion_r553519738
##########
File path: helix-core/src/main/java/org/apache/helix/task/WorkflowContext.java
##########
@@ -48,22 +48,26 @@
LAST_PURGE_TIME,
StartTime, // TODO this should be named JOB_SCHEDULED_START_TIME, it's not the actual start time of the job
NAME
- }
+ }
public static final int NOT_STARTED = -1;
public static final int UNFINISHED = -1;
+ // Note: This field needs to be set if any of the workflow context fields have been changed.
+ // Otherwise, the context will not be written to ZK by the controller.
+ private boolean isModified;
+
public WorkflowContext(ZNRecord record) {
super(record);
+ isModified = false;
}
public void setWorkflowState(TaskState s) {
String workflowState = _record.getSimpleField(WorkflowContextProperties.STATE.name());
- if (workflowState == null) {
- _record.setSimpleField(WorkflowContextProperties.STATE.name(), s.name());
- } else if (!workflowState.equals(TaskState.FAILED.name()) && !workflowState
- .equals(TaskState.COMPLETED.name())) {
+ if (workflowState == null || (!workflowState.equals(TaskState.FAILED.name())
Review comment:
Done.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] alirezazamani commented on a change in pull request #1600: Eliminate redundant workflow context writes
Posted by GitBox <gi...@apache.org>.
alirezazamani commented on a change in pull request #1600:
URL: https://github.com/apache/helix/pull/1600#discussion_r553528699
##########
File path: helix-core/src/main/java/org/apache/helix/task/WorkflowContext.java
##########
@@ -200,23 +222,37 @@ public long getFinishTime() {
}
public void setLastScheduledSingleWorkflow(String workflow) {
- _record.setSimpleField(WorkflowContextProperties.LAST_SCHEDULED_WORKFLOW.name(), workflow);
+ String lastScheduleWorkflow =
+ _record.getSimpleField(WorkflowContextProperties.LAST_SCHEDULED_WORKFLOW.name());
+ if (!workflow.equals(lastScheduleWorkflow)) {
+ _record.setSimpleField(WorkflowContextProperties.LAST_SCHEDULED_WORKFLOW.name(), workflow);
+ markWorkflowContextAsModified();
+ }
// Record scheduled workflow into the history list as well
- List<String> workflows = getScheduledWorkflows();
- if (workflows == null) {
- workflows = new ArrayList<String>();
- _record.setListField(WorkflowContextProperties.SCHEDULED_WORKFLOWS.name(), workflows);
+ List<String> scheduledWorkflows = getScheduledWorkflows();
+ if (scheduledWorkflows == null) {
+ scheduledWorkflows = new ArrayList<String>();
+ _record.setListField(WorkflowContextProperties.SCHEDULED_WORKFLOWS.name(),
+ scheduledWorkflows);
+ markWorkflowContextAsModified();
+ }
+ if (!scheduledWorkflows.contains(workflow)) {
Review comment:
Done.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] alirezazamani commented on a change in pull request #1600: Eliminate redundant workflow context writes
Posted by GitBox <gi...@apache.org>.
alirezazamani commented on a change in pull request #1600:
URL: https://github.com/apache/helix/pull/1600#discussion_r552933307
##########
File path: helix-core/src/main/java/org/apache/helix/task/WorkflowContext.java
##########
@@ -48,22 +48,27 @@
LAST_PURGE_TIME,
StartTime, // TODO this should be named JOB_SCHEDULED_START_TIME, it's not the actual start time of the job
NAME
- }
+ }
public static final int NOT_STARTED = -1;
public static final int UNFINISHED = -1;
+ private boolean isModified;
Review comment:
Thanks for the coment. Done.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] alirezazamani commented on a change in pull request #1600: Eliminate redundant workflow context writes
Posted by GitBox <gi...@apache.org>.
alirezazamani commented on a change in pull request #1600:
URL: https://github.com/apache/helix/pull/1600#discussion_r553013982
##########
File path: helix-core/src/main/java/org/apache/helix/task/WorkflowContext.java
##########
@@ -228,10 +265,22 @@ public long getLastJobPurgeTime() {
}
public void setName(String name) {
- _record.setSimpleField(WorkflowContextProperties.NAME.name(), name);
+ String oldName = getName();
+ if (oldName == null || !oldName.equals(name)) {
Review comment:
Done.
##########
File path: helix-core/src/main/java/org/apache/helix/task/WorkflowContext.java
##########
@@ -48,22 +48,27 @@
LAST_PURGE_TIME,
StartTime, // TODO this should be named JOB_SCHEDULED_START_TIME, it's not the actual start time of the job
NAME
- }
+ }
public static final int NOT_STARTED = -1;
public static final int UNFINISHED = -1;
+ private boolean isModified;
+
public WorkflowContext(ZNRecord record) {
super(record);
+ isModified = false;
}
public void setWorkflowState(TaskState s) {
String workflowState = _record.getSimpleField(WorkflowContextProperties.STATE.name());
if (workflowState == null) {
Review comment:
Done.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] alirezazamani commented on pull request #1600: Eliminate redundant workflow context writes
Posted by GitBox <gi...@apache.org>.
alirezazamani commented on pull request #1600:
URL: https://github.com/apache/helix/pull/1600#issuecomment-755723881
> We should use the result of github run as standard now. Does it pass now?
It does not even run recently.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] alirezazamani merged pull request #1600: Eliminate redundant workflow context writes
Posted by GitBox <gi...@apache.org>.
alirezazamani merged pull request #1600:
URL: https://github.com/apache/helix/pull/1600
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org
[GitHub] [helix] NealSun96 commented on a change in pull request #1600: Eliminate redundant workflow context writes
Posted by GitBox <gi...@apache.org>.
NealSun96 commented on a change in pull request #1600:
URL: https://github.com/apache/helix/pull/1600#discussion_r552914334
##########
File path: helix-core/src/main/java/org/apache/helix/task/WorkflowContext.java
##########
@@ -48,22 +48,27 @@
LAST_PURGE_TIME,
StartTime, // TODO this should be named JOB_SCHEDULED_START_TIME, it's not the actual start time of the job
NAME
- }
+ }
public static final int NOT_STARTED = -1;
public static final int UNFINISHED = -1;
+ private boolean isModified;
Review comment:
Somewhere in the class, you need to note that `isModified` need to be updated every time there's a change. Future developers may forget and break this.
##########
File path: helix-core/src/main/java/org/apache/helix/task/WorkflowContext.java
##########
@@ -48,22 +48,27 @@
LAST_PURGE_TIME,
StartTime, // TODO this should be named JOB_SCHEDULED_START_TIME, it's not the actual start time of the job
NAME
- }
+ }
public static final int NOT_STARTED = -1;
public static final int UNFINISHED = -1;
+ private boolean isModified;
+
public WorkflowContext(ZNRecord record) {
super(record);
+ isModified = false;
}
public void setWorkflowState(TaskState s) {
String workflowState = _record.getSimpleField(WorkflowContextProperties.STATE.name());
if (workflowState == null) {
Review comment:
Shouldn't this entire if statement be collapsed into one block?
##########
File path: helix-core/src/main/java/org/apache/helix/task/WorkflowContext.java
##########
@@ -228,10 +265,22 @@ public long getLastJobPurgeTime() {
}
public void setName(String name) {
- _record.setSimpleField(WorkflowContextProperties.NAME.name(), name);
+ String oldName = getName();
+ if (oldName == null || !oldName.equals(name)) {
Review comment:
Those checks could in general be consolidated to `!name.equals(oldName)`.
----------------------------------------------------------------
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
---------------------------------------------------------------------
To unsubscribe, e-mail: reviews-unsubscribe@helix.apache.org
For additional commands, e-mail: reviews-help@helix.apache.org