szetszwo commented on code in PR #7418:
URL: https://github.com/apache/ozone/pull/7418#discussion_r1847591395
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/transport/server/ratis/ContainerStateMachine.java:
##########
@@ -265,6 +268,23 @@ public ContainerStateMachine(HddsDatanodeService
hddsDatanodeService, RaftGroupI
}
+ private void validatePeers(RaftServer server, RaftGroupId id) throws
IOException {
Review Comment:
Do not pass member fields/methods. Get them inside `validatePeers`.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/transport/server/ratis/ContainerStateMachine.java:
##########
@@ -265,6 +268,23 @@ public ContainerStateMachine(HddsDatanodeService
hddsDatanodeService, RaftGroupI
}
+ private void validatePeers(RaftServer server, RaftGroupId id) throws
IOException {
+ if (this.peersValidated.get()) {
+ return;
+ }
+ RaftPeerId selfId = server.getId();
+ Collection<RaftPeer> peers = server.getDivision(id).getGroup().getPeers();
+ // If peers list is empty then it means Ratis hasn't created any
raft--meta file containing the last applied
+ // transaction. Then the peer list can be only validated on apply
transaction.
Review Comment:
Peer list cannot be empty here. Remove the check.
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/transport/server/ratis/ContainerStateMachine.java:
##########
@@ -265,6 +268,23 @@ public ContainerStateMachine(HddsDatanodeService
hddsDatanodeService, RaftGroupI
}
+ private void validatePeers(RaftServer server, RaftGroupId id) throws
IOException {
+ if (this.peersValidated.get()) {
+ return;
+ }
+ RaftPeerId selfId = server.getId();
+ Collection<RaftPeer> peers = server.getDivision(id).getGroup().getPeers();
+ // If peers list is empty then it means Ratis hasn't created any
raft--meta file containing the last applied
+ // transaction. Then the peer list can be only validated on apply
transaction.
+ if (!peers.isEmpty() && peers.stream().noneMatch(raftPeer -> raftPeer !=
null
+ && raftPeer.getId().equals(selfId))) {
+ throw new StorageContainerException(String.format("Current datanodeId:
%s is not part of the " +
+ "group : %s with quorum: %s", selfId, id, peers),
ContainerProtos.Result.INVALID_CONFIG);
+ } else if (!peers.isEmpty()) {
+ peersValidated.set(true);
+ }
Review Comment:
This can be simplified as below:
```java
private void validatePeers() throws IOException {
if (peersValidated.get()) {
return;
}
final RaftGroup group =
ratisServer.getServerDivision(getGroupId()).getGroup();
final RaftPeerId selfId = ratisServer.getServer().getId();
if (group.getPeer(selfId) == null) {
throw new StorageContainerException("Current datanode " + selfId + "
is not a member of " + group,
ContainerProtos.Result.INVALID_CONFIG);
}
peersValidated.set(true);
}
```
##########
hadoop-hdds/container-service/src/main/java/org/apache/hadoop/ozone/container/common/transport/server/ratis/ContainerStateMachine.java:
##########
@@ -962,6 +982,11 @@ private CompletableFuture<ContainerCommandResponseProto>
applyTransaction(
final CheckedSupplier<ContainerCommandResponseProto, Exception> task
= () -> {
try {
+ try {
+ this.validatePeers(this.ratisServer.getServer(), getGroupId());
+ } catch (StorageContainerException e) {
+ return ContainerUtils.logAndReturnError(LOG, e, request);
+ }
Review Comment:
What is the expectation? If the peer is not in the group, fail it?
If it is the case, we should do it earlier. Why not doing it in
`startTransaction(LogEntryProto, RaftPeerRole)`?
--
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]
