szetszwo commented on code in PR #1154:
URL: https://github.com/apache/ratis/pull/1154#discussion_r1775579701
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/LeaderStateImpl.java:
##########
@@ -219,11 +219,11 @@ void addAll(Collection<LogAppender> newSenders) {
Preconditions.assertTrue(changed);
}
- boolean removeAll(Collection<LogAppender> c) {
+ synchronized boolean removeAll(Collection<LogAppender> c) {
return senders.removeAll(c);
}
- CompletableFuture<Void> stopAll() {
+ synchronized CompletableFuture<Void> stopAll() {
Review Comment:
Since it is a `CopyOnWriteArrayList`, we should use `iterator()` or
`stream()` instead of `get(i)`, i.e.
```java
CompletableFuture<Void> stopAll() {
return
CompletableFuture.allOf(senders.stream().map(LogAppender::stopAsync).toArray(CompletableFuture[]::new));
}
```
Then, we don't need `synchronized` for all the methods in `SenderList`.
##########
ratis-server/src/main/java/org/apache/ratis/server/leader/InstallSnapshotRequests.java:
##########
@@ -143,11 +143,9 @@ public InstallSnapshotRequestProto next() {
}
private InstallSnapshotRequestProto
newInstallSnapshotRequest(FileChunkProto chunk, boolean done) {
- synchronized (server) {
- final SnapshotChunkProto.Builder b =
LeaderProtoUtils.toSnapshotChunkProtoBuilder(
- requestId, requestIndex++, snapshot.getTermIndex(), chunk,
totalSize, done);
- return LeaderProtoUtils.toInstallSnapshotRequestProto(server,
followerId, b);
- }
+ final SnapshotChunkProto.Builder b =
LeaderProtoUtils.toSnapshotChunkProtoBuilder(
+ requestId, requestIndex++, snapshot.getTermIndex(), chunk,
totalSize, done);
+ return LeaderProtoUtils.toInstallSnapshotRequestProto(server,
followerId, b);
Review Comment:
Let's keep `synchronized (server)`. Otherwise, it could create an
inconsistent request.
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/ConfigurationManager.java:
##########
@@ -78,7 +78,7 @@ private void addRaftConfigurationImpl(long logIndex,
RaftConfigurationImpl conf)
}
}
- RaftConfigurationImpl getCurrent() {
+ synchronized RaftConfigurationImpl getCurrent() {
return currentConf;
}
Review Comment:
Let's also add `synchronized` to `getCurrentPeer()`. Then, all the
non-private methods are synchronized. We should remove `volatile`.
```diff
@@ -41,11 +41,9 @@ public class ConfigurationManager {
* The current raft configuration. If configurations is not empty, should
be
* the last entry of the map. Otherwise is initialConf.
*/
- @SuppressWarnings({"squid:S3077"}) // Suppress volatile for generic type
- private volatile RaftConfigurationImpl currentConf;
+ private RaftConfigurationImpl currentConf;
/** Cache the peer corresponding to {@link #id}. */
- @SuppressWarnings({"squid:S3077"}) // Suppress volatile for generic type
- private volatile RaftPeer currentPeer;
+ private RaftPeer currentPeer;
ConfigurationManager(RaftPeerId id, RaftConfigurationImpl initialConf) {
this.id = id;
@@ -78,11 +76,11 @@ public class ConfigurationManager {
}
}
- RaftConfigurationImpl getCurrent() {
+ synchronized RaftConfigurationImpl getCurrent() {
return currentConf;
}
- RaftPeer getCurrentPeer() {
+ synchronized RaftPeer getCurrentPeer() {
return currentPeer;
}
```
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/ServerState.java:
##########
@@ -58,7 +58,7 @@
* Common states of a raft peer. Protected by RaftServer's lock.
*/
class ServerState {
- private final RaftGroupMemberId memberId;
+ private final AtomicReference<RaftGroupMemberId> memberId = new
AtomicReference<>();
Review Comment:
Let's don't add `AtomicReference` since `memberId` is never changed.
##########
ratis-server/src/main/java/org/apache/ratis/server/impl/RoleInfo.java:
##########
@@ -47,7 +47,7 @@ class RoleInfo {
public static final Logger LOG = LoggerFactory.getLogger(RoleInfo.class);
private final RaftPeerId id;
- private volatile RaftPeerRole role;
+ private final AtomicReference<RaftPeerRole> role = new AtomicReference<>();
Review Comment:
`volatile` and `AtomicReference` seems the same for this case?
##########
ratis-server/src/main/java/org/apache/ratis/server/leader/LogAppenderBase.java:
##########
@@ -128,7 +128,7 @@ public void start() {
@Override
public boolean isRunning() {
- return daemon.isWorking();
+ return daemon.isWorking() && server.getInfo().isLeader();
Review Comment:
This is a good change!
--
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]
