NealSun96 commented on code in PR #2102:
URL: https://github.com/apache/helix/pull/2102#discussion_r883118880
##########
helix-core/src/main/java/org/apache/helix/task/AssignableInstanceManager.java:
##########
@@ -419,7 +425,9 @@ public void updateAssignableInstances(ClusterConfig
clusterConfig,
}
LOG.info(
"AssignableInstanceManager updated AssignableInstances due to
LiveInstance/InstanceConfig change.");
-
+ if (_assignableInstanceMap.isEmpty()) {
+ LOG.warn("No assignable instance!");
Review Comment:
Synced offline: these can be removed.
##########
helix-core/src/main/java/org/apache/helix/task/assigner/ThreadCountBasedTaskAssigner.java:
##########
@@ -74,19 +75,16 @@ public Map<String, TaskAssignResult>
assignTasks(Iterable<AssignableInstance> as
public Map<String, TaskAssignResult> assignTasks(
AssignableInstanceManager assignableInstanceManager, Collection<String>
instances,
Iterable<TaskConfig> tasks, String quotaType) {
- Iterable<AssignableInstance> assignableInstances = new HashSet<>();
+ Set<AssignableInstance> assignableInstances = new HashSet<>();
// Only add the AssignableInstances that are also in instances
for (String instance : instances) {
- ((HashSet<AssignableInstance>) assignableInstances)
- .add(assignableInstanceManager.getAssignableInstance(instance));
+
assignableInstances.add(assignableInstanceManager.getAssignableInstance(instance));
Review Comment:
Synced offline: this is a NIT.
##########
helix-core/src/main/java/org/apache/helix/task/WorkflowDispatcher.java:
##########
@@ -150,7 +149,10 @@ && isTimeout(workflowCtx.getStartTime(),
workflowCfg.getTimeout())) {
// For workflows that have already reached final states, STOP should not
take into effect.
if (!TaskConstants.FINAL_STATES.contains(workflowCtx.getWorkflowState())
&& TargetState.STOP.equals(targetState)) {
- LOG.info("Workflow {} is marked as stopped. Workflow state is {}",
workflow,
+ if (workflowCtx.getWorkflowState() == TaskState.STOPPED) {
+ return;
+ }
+ LOG.debug("Workflow {} is marked as stopped. Workflow state is {}",
workflow,
workflowCtx.getWorkflowState());
if (isWorkflowStopped(workflowCtx, workflowCfg)) {
workflowCtx.setWorkflowState(TaskState.STOPPED);
Review Comment:
Chatted offline, @qqu0127 suggested that cases exist where the if statement
on line 151 passes but not the one on line 155, resulting in an immediate
return; such a case doesn't allow combing the if conditions as I suggested.
I change my suggestion to
```
if (!TaskState.STOPPED.equals(workflowCtx.getWorkflowState()) &&
isWorkflowStopped(workflowCtx, workflowCfg)) {
LOG.debug(...)
```
This would also fix the problem of log being misplaced.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
