jiajunwang commented on a change in pull request #1068:
URL: https://github.com/apache/helix/pull/1068#discussion_r437058553
##########
File path:
helix-core/src/main/java/org/apache/helix/controller/stages/MessageGenerationPhase.java
##########
@@ -220,7 +220,8 @@ private void generateMessage(final Resource resource, final
BaseControllerDataPr
generateCancellationMessageForPendingMessage(desiredState,
currentState, nextState, pendingMessage,
manager, resource, partition, sessionIdMap, instanceName,
stateModelDef,
cancellationMessage, isCancellationEnabled);
- } else {
+ }
+ if (pendingMessage == null ||
currentState.equalsIgnoreCase(pendingMessage.getToState())) {
Review comment:
When "pendingMessage != null" but
"currentState.equalsIgnoreCase(pendingMessage.getToState())", the cancellation
message will be replaced with the new ST message right? I think it works. But
would it be better to send both of the cancellation message and the new one?
If we do this, will the participant discards the cancellation message, or
proceed with both of them? It would be safer from Helix perspective since the
pending message will be removed for sure with cancellation. But it could be
complicated from the participant perspective since I think we might need some
sync control on the participant side.
##########
File path:
helix-core/src/main/java/org/apache/helix/controller/stages/MessageGenerationPhase.java
##########
@@ -220,7 +220,8 @@ private void generateMessage(final Resource resource, final
BaseControllerDataPr
generateCancellationMessageForPendingMessage(desiredState,
currentState, nextState, pendingMessage,
manager, resource, partition, sessionIdMap, instanceName,
stateModelDef,
cancellationMessage, isCancellationEnabled);
- } else {
+ }
+ if (pendingMessage == null ||
currentState.equalsIgnoreCase(pendingMessage.getToState())) {
Review comment:
Then it means the decision of canceling or not is purely in the
generateCancellationMessageForPendingMessage(). And we don't have the option to
send both of the messages for now.
In this case, it would be cleaner to do the following, I guess.
```
if (pendingMessage != null) {
message =
generateCancellationMessageForPendingMessage(....);
}
if (message == null) {
// Create new state transition message
message = createStateTransitionMessage(...);
}
```
##########
File path:
helix-core/src/main/java/org/apache/helix/controller/stages/MessageGenerationPhase.java
##########
@@ -220,7 +220,8 @@ private void generateMessage(final Resource resource, final
BaseControllerDataPr
generateCancellationMessageForPendingMessage(desiredState,
currentState, nextState, pendingMessage,
manager, resource, partition, sessionIdMap, instanceName,
stateModelDef,
cancellationMessage, isCancellationEnabled);
- } else {
+ }
+ if (pendingMessage == null ||
currentState.equalsIgnoreCase(pendingMessage.getToState())) {
Review comment:
Then it means the decision of canceling or not is purely in the
generateCancellationMessageForPendingMessage(). And we don't have the option to
send both of the messages for now.
##########
File path:
helix-core/src/main/java/org/apache/helix/controller/stages/MessageGenerationPhase.java
##########
@@ -220,7 +220,8 @@ private void generateMessage(final Resource resource, final
BaseControllerDataPr
generateCancellationMessageForPendingMessage(desiredState,
currentState, nextState, pendingMessage,
manager, resource, partition, sessionIdMap, instanceName,
stateModelDef,
cancellationMessage, isCancellationEnabled);
- } else {
+ }
+ if (pendingMessage == null ||
currentState.equalsIgnoreCase(pendingMessage.getToState())) {
Review comment:
I was thinking if there is a way to avoid the same condition being
checked inside generateCancellationMessageForPendingMessage and here. But it
seems that there is no easy way.
However, to avoid the possible unexpected overwrite (for instance, if we
change the private method logic later), I think we can change as following,
Option one,
`if (message == null && (pendingMessage == null ||
currentState.equalsIgnoreCase(pendingMessage.getToState()))) {`
Which means if there is no cancellation message and the the pending message
has been done then we generate new ST.
Option two,
Remove generateCancellationMessageForPendingMessage and move all if-else
conditions back to the caller methods. Because you do need to handle those
conditions with different logic. That seems to be a clearer logic.
##########
File path:
helix-core/src/main/java/org/apache/helix/controller/stages/MessageGenerationPhase.java
##########
@@ -220,7 +220,8 @@ private void generateMessage(final Resource resource, final
BaseControllerDataPr
generateCancellationMessageForPendingMessage(desiredState,
currentState, nextState, pendingMessage,
manager, resource, partition, sessionIdMap, instanceName,
stateModelDef,
cancellationMessage, isCancellationEnabled);
- } else {
+ }
+ if (pendingMessage == null ||
currentState.equalsIgnoreCase(pendingMessage.getToState())) {
Review comment:
I think the other place will never hit the other 2 conditions. But not
100% sure.
Anyway, both options work.
----------------------------------------------------------------
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:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
