jiajunwang commented on a change in pull request #1816:
URL: https://github.com/apache/helix/pull/1816#discussion_r669972905
##########
File path:
helix-core/src/main/java/org/apache/helix/controller/stages/ManagementMessageGenerationPhase.java
##########
@@ -96,7 +96,9 @@ public void process(ClusterEvent event) throws Exception {
LiveInstance liveInstance = liveInstanceMap.get(instanceName);
Collection<Message> pendingMessages =
allInstanceMessages.get(instanceName);
String sessionId = liveInstance.getEphemeralOwner();
- LiveInstanceStatus currentStatus = liveInstance.getStatus();
+ LiveInstanceStatus liveInstanceStatus = liveInstance.getStatus();
Review comment:
There was no convention about how a HelixProperty class should handle
such a case (missing field). So we have both types of examples through our
codebase. For the future, I think we should make all these different cases
aligned.
1. From my perspective, each HelixProperty class should carry more business
logic. And let the ZNode class to handle the raw content. So missing field can
be returned from ZNode or it's methods. But should not be returned by the
HelixProperty unless it is representing a specific business logic.
2. Based on point one, do you think null LiveInstanceStatus is representing
any business logic? I think return NORMAL when this field is missing would be
quite straightforward and is not confusing at all. Moreover, it is fully
backward compatible. The older participant does not have any special status, so
they will be NORMAL forever.
--
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]
