pkuwm commented on a change in pull request #1066:
URL: https://github.com/apache/helix/pull/1066#discussion_r436147672
##########
File path: helix-core/src/main/java/org/apache/helix/HelixManager.java
##########
@@ -425,6 +426,19 @@ void
addExternalViewChangeListener(org.apache.helix.ExternalViewChangeListener l
*/
boolean isLeader();
+ /**
+ * Checks if the cluster manager is leader and sets its ZK session in
+ * {@link ControllerLeaderSession}.
+ *
+ * @param controllerLeaderSession To include ZK session of the cluster
manager in return
+ *
+ * @return true if this is a controller and a leader of the cluster. Zk
session of the cluster
+ * manager is set in controllerLeaderSession
+ */
+ default boolean isLeader(ControllerLeaderSession controllerLeaderSession) {
Review comment:
> Got it. Thanks for pointing that out. I will get a better name for it.
Maybe "ZkSessionWrapper". If you have a better name, please let me know.
> […](#)
> On Fri, Jun 5, 2020 at 12:52 PM Lei Xia ***@***.***> wrote: ***@***.****
commented on this pull request. ------------------------------ In
helix-core/src/main/java/org/apache/helix/HelixManager.java <[#1066
(comment)](https://github.com/apache/helix/pull/1066#discussion_r436132217)>: >
@@ -425,6 +426,19 @@ void
addExternalViewChangeListener(org.apache.helix.ExternalViewChangeListener l */
boolean isLeader(); + /** + * Checks if the cluster manager is leader and sets
its ZK session in + * ***@***.*** ControllerLeaderSession}. + * + * @param
controllerLeaderSession To include ZK session of the cluster manager in return
+ * + * @return true if this is a controller and a leader of the cluster. Zk
session of the cluster + * manager is set in controllerLeaderSession + */ +
default boolean isLeader(ControllerLeaderSession controllerLeaderSession) {
> This could be confusion because there is no "leader" concept for a Helix
manager. Leader only applies to a controller instance, but HelixManager is a
broader concept here.
Thinking in another aspect, the java doc also confuses. That's why I call it
`ControllerLeaderSession`. Though we don't have a "leader" concept for helix
manager, but a helix manager could be attached to a controller which could be a
leader. And this is already public API, we could not change the name of the old
API now. For this new one, maybe we could think of a new name. Maybe like
`isInstanceLeader()`?
Let me also fix the java doc to avoid any confusion.
----------------------------------------------------------------
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]
