szetszwo commented on code in PR #673:
URL: https://github.com/apache/ratis/pull/673#discussion_r919568414
##########
ratis-common/src/main/java/org/apache/ratis/protocol/RaftPeer.java:
##########
@@ -17,6 +17,7 @@
*/
package org.apache.ratis.protocol;
+import org.apache.ratis.proto.RaftProtos;
Review Comment:
Change the import to
```
import org.apache.ratis.proto.RaftProtos.RaftPeerRole;
```
It will make the code easier to read.
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderStateImpl.java:
##########
@@ -951,9 +951,11 @@ private void yieldLeaderToHigherPriorityPeer() {
for (LogAppender logAppender : senders.getSenders()) {
final FollowerInfo followerInfo = logAppender.getFollower();
final RaftPeerId followerID = followerInfo.getPeer().getId();
- final RaftPeer follower = conf.getPeer(followerID);
+ final RaftPeer follower = conf.getPeer(followerID,
RaftPeerRole.FOLLOWER);
if (follower == null) {
- LOG.error("{} the follower {} is not in the conf {}", this,
server.getId(), conf);
+ if (conf.getPeer(followerID, RaftPeerRole.LISTENER) == null) {
+ LOG.error("{} the follower {} is not in the conf {}", this,
server.getId(), conf);
Review Comment:
The existing code server.getId() was incorrect. It should use followerID.
```
LOG.error("{} the follower {} is not in the conf {}", this,
followerID, conf);
```
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/PeerConfiguration.java:
##########
@@ -97,7 +97,7 @@ int size() {
@Override
public String toString() {
- return peers.values().toString();
+ return "peers:" + peers.values() + "|listeners:" + listeners.toString();
Review Comment:
It should be `listeners.values()`.
##########
ratis-server-api/src/main/java/org/apache/ratis/server/RaftServer.java:
##########
@@ -77,7 +80,14 @@ default RaftPeer getPeer() {
/** @return the {@link RaftGroup} for this division. */
default RaftGroup getGroup() {
- return RaftGroup.valueOf(getMemberId().getGroupId(),
getRaftConf().getAllPeers());
+ Collection<RaftPeer> allFollowerPeers =
+ getRaftConf().getAllPeers(RaftProtos.RaftPeerRole.FOLLOWER);
+ Collection<RaftPeer> allListenerPeers =
+ getRaftConf().getAllPeers(RaftProtos.RaftPeerRole.LISTENER);
+ Iterable<RaftPeer> peers = Iterables.concat(allFollowerPeers,
allListenerPeers);
+ return RaftGroup.valueOf(
+ getMemberId().getGroupId(),
+ Lists.newArrayList(peers));
Review Comment:
Change the `peers` parameter of `RaftGroup.valueOf(..)` to
`Iterable<RaftPeer>`. Then, we don't have to create a new array list.
```
default RaftGroup getGroup() {
final Collection<RaftPeer> followers =
getRaftConf().getAllPeers(RaftPeerRole.FOLLOWER);
final Collection<RaftPeer> listeners =
getRaftConf().getAllPeers(RaftPeerRole.LISTENER);
final Iterable<RaftPeer> peers = Iterables.concat(followers,
listeners);
return RaftGroup.valueOf(getMemberId().getGroupId(), peers);
}
```
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/RaftServerImpl.java:
##########
@@ -494,10 +494,10 @@ public void close() {
*/
private synchronized boolean changeToFollower(long newTerm, boolean force,
Object reason) {
final RaftPeerRole old = role.getCurrentRole();
+ final boolean metadataUpdated = state.updateCurrentTerm(newTerm);
if (old == RaftPeerRole.LISTENER) {
- throw new IllegalStateException("Unexpected role " + old);
+ return metadataUpdated;
}
- final boolean metadataUpdated = state.updateCurrentTerm(newTerm);
Review Comment:
Let's add a new parameter `allowListener`. We will allow listener in
appendEntires and installSnapshot but not requestVote.
```
@@ -494,9 +494,9 @@ class RaftServerImpl implements RaftServer.Division,
* @param force Force to start a new {@link FollowerState} even if this
server is already a follower.
* @return if the term/votedFor should be updated to the new term
*/
- private synchronized boolean changeToFollower(long newTerm, boolean
force, Object reason) {
+ private synchronized boolean changeToFollower(long newTerm, boolean
force, boolean allowListener, Object reason) {
final RaftPeerRole old = role.getCurrentRole();
- if (old == RaftPeerRole.LISTENER) {
+ if (old == RaftPeerRole.LISTENER && !allowListener) {
throw new IllegalStateException("Unexpected role " + old);
}
final boolean metadataUpdated = state.updateCurrentTerm(newTerm);
```
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderStateImpl.java:
##########
@@ -951,9 +951,11 @@ private void yieldLeaderToHigherPriorityPeer() {
for (LogAppender logAppender : senders.getSenders()) {
final FollowerInfo followerInfo = logAppender.getFollower();
final RaftPeerId followerID = followerInfo.getPeer().getId();
- final RaftPeer follower = conf.getPeer(followerID);
+ final RaftPeer follower = conf.getPeer(followerID,
RaftPeerRole.FOLLOWER);
Review Comment:
Listener is a new feature. The getPeer() method is for backward
compatibility since it is in ratis-server-api.
--
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]
