JAkutenshi commented on code in PR #4090:
URL: https://github.com/apache/ignite-3/pull/4090#discussion_r1705543214
##########
modules/replicator/src/main/java/org/apache/ignite/internal/replicator/ReplicaManager.java:
##########
@@ -295,7 +295,7 @@ public ReplicaManager(
Executor requestsExecutor,
LongSupplier idleSafeTimePropagationPeriodMsSupplier,
FailureProcessor failureProcessor,
- Marshaller raftCommandsMarshaller,
+ @Nullable Marshaller raftCommandsMarshaller,
Review Comment:
Previously there were empty snapshot storage implementation, but later I
found that there are places where `RaftGroupOptions#defaults` is used and all
discussed fields are handled with null-checks like in
`org.apache.ignite.internal.raft.server.RaftGroupOptions#snapshotStorageFactory()`
lines 480-482 for marshaller and 493-495 for storage factory. There is a point
why I had been decided to use already existed mechanisms.
On the other hand in tests there several (mostly dependencies) issues with
especially the factory and marshaller, on of the most known is [IGNITE-22222
with thread local marshaller in a
test](https://issues.apache.org/jira/browse/IGNITE-22222). With the current
solution we're reusing internal raft code and rely on it and get rid of
dependency issues in tests.
>only commandsMarshaller is marked as https://github.com/nullable, but
snapshotStorageFactory doesn't.
About that, we can look inside `RaftGroupOptions` and see that fields
inconsistently marked as `@Nullable`, but they aren't initializing in the
constructor and the only way to set the value is setters that may not be
called. The most fair way is to mark the snapshots factory as nullable too and
remove null checks in `ReplicaManager`, relying totally on raft internals.
--
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]
