ibessonov commented on code in PR #7361:
URL: https://github.com/apache/ignite-3/pull/7361#discussion_r2726961000
##########
modules/raft/src/main/java/org/apache/ignite/internal/raft/server/impl/JraftServerImpl.java:
##########
@@ -957,10 +962,10 @@ public void onSnapshotSave(SnapshotWriter writer, Closure
done) {
@Override
public boolean onSnapshotLoad(SnapshotReader reader) {
try {
- boolean result = false;
+ boolean result = true;
for (RaftGroupListener listener : listeners) {
- result =
listener.onSnapshotLoad(Path.of(reader.getPath()));
+ result =
listener.onSnapshotLoad(Path.of(reader.getPath())) && result;
Review Comment:
Please rewrite this code so that an unconditional execution of
`onSnapshotLoad` is explicit and does not depend on the order of arguments in
this expression, thank you!
##########
modules/raft/src/main/java/org/apache/ignite/internal/raft/server/impl/JraftServerImpl.java:
##########
@@ -920,27 +928,29 @@ private static List<String>
peersIdsToStrings(Collection<PeerId> peerIds) {
@Override
public void onSnapshotSave(SnapshotWriter writer, Closure done) {
try {
- listener.onSnapshotSave(Path.of(writer.getPath()), res -> {
- if (res == null) {
- File file = new File(writer.getPath());
-
- File[] snapshotFiles = file.listFiles();
-
- // Files array can be null if shanpshot folder doesn't
exist.
- if (snapshotFiles != null) {
- for (File file0 : snapshotFiles) {
- if (file0.isFile()) {
- writer.addFile(file0.getName(), null);
+ for (RaftGroupListener listener : listeners) {
+ listener.onSnapshotSave(Path.of(writer.getPath()), res -> {
Review Comment:
Ok, now this code looks wrong. I believe we need a separate class of
listeners for `onLeaderStart/onLeaderStop` that is not a raft group listener
##########
modules/raft/src/main/java/org/apache/ignite/raft/jraft/rpc/impl/ActionRequestProcessor.java:
##########
@@ -100,6 +100,14 @@ public final void handleRequest(RpcContext rpcCtx,
ActionRequest request) {
protected void handleRequestInternal(RpcContext rpcCtx, Node node,
ActionRequest request, Marshaller commandsMarshaller) {
DelegatingStateMachine fsm = (DelegatingStateMachine)
node.getOptions().getFsm();
+ BeforeApplyHandler beforeApplyHandler = null;
+ for (RaftGroupListener lsnr : fsm.getListeners()) {
+ if (lsnr instanceof BeforeApplyHandler) {
+ beforeApplyHandler = (BeforeApplyHandler) lsnr;
Review Comment:
This part looks dangerous, I would assume that we should use all
`BeforeApplyHandler`s, not just the first one that we find.
Why did you remove the loop? Why did it have a `break`?
--
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]
