Github user hanm commented on a diff in the pull request:
https://github.com/apache/zookeeper/pull/628#discussion_r221416332
--- Diff: src/java/main/org/apache/zookeeper/server/quorum/Learner.java ---
@@ -232,8 +236,8 @@ protected void sockConnect(Socket sock,
InetSocketAddress addr, int timeout)
}
/**
- * Establish a connection with the Leader found by findLeader. Retries
- * until either initLimit time has elapsed or 5 tries have happened.
+ * Establish a connection with the LearnerMaster found by
findLearnerMaster.
+ * Retries until either initLimit time has elapsed or 5 tries have
happened.
--- End diff --
`connectToLeader` is still used by Followers to connect to leader, so this
comment should be updated. Maybe leaving the old comment, something like
`Establish a connection with the Leader found by findLeader, or with the
LearnerMaster found by findLearnerMaster`?
Even so the name of the function `connectToLeader` would be confusing as
it's now overloaded to connect to two types of host (leader or learnerMaster).
Same issue existing on `syncWithLeader`, when an observer connects to a
follower the observer will be syncing from follower instead of leader but the
method name. It's confusing.
One approach is to have separate methods such as `connectToLearnerMaster`
and `syncWithLearnerMaster` or `syncWithFollower`, which will make the semantic
explicit and clear. Or, we could make the existing methods name more generic -
e.g. `connectToServer`, `syncWithServer`. I think the latter is tempting.
Thoughts?
---
