Cyrill commented on code in PR #4005:
URL: https://github.com/apache/ignite-3/pull/4005#discussion_r1712611954
##########
modules/raft-api/src/main/java/org/apache/ignite/internal/raft/RaftManager.java:
##########
@@ -118,7 +124,8 @@ <T extends RaftGroupService> CompletableFuture<T>
startRaftGroupNodeAndWaitNodeR
RaftGroupListener lsnr,
RaftGroupEventsListener eventsLsnr,
RaftNodeDisruptorConfiguration disruptorConfiguration,
- RaftServiceFactory<T> factory
+ RaftServiceFactory<T> factory,
+ RaftOptionsConfigurator storageConfigurator
Review Comment:
I agree this looks broken, but fixing it requires much more changes. There
is already a separate task for it, IGNITE-18273.
RaftServiceFactory is used in a different manner to replace the default
implementation of `RaftGroupService`, not `LogStorageFactory` (which is related
to raft node):
```
raftServiceFactory == null
? (CompletableFuture<T>)
startRaftGroupServiceInternal(nodeId.groupId(), configuration, cmdMarshaller)
: raftServiceFactory.startRaftGroupService(nodeId.groupId(),
configuration, raftConfiguration, executor, cmdMarshaller);
```
For each method in `RaftManager` there is a twin method that sets this
factory.
The main issue here is that `ignite-api` has neither the notion of
`LogStorageFactory` nor `RaftOptions` (which is expected, as these two are
implementation details).
Additionally there are already a couple of places where the api is casted to
the implementation, so it's a red flag the API is not quite complete:
```
((Loza) raftManager).startRaftGroupNode(...);
```
To sum up, the hierarchy and the API of `raft-api` - `raft` needs revising
and it might require a significant amount of changes, so I believe it need to
be done in a separate task.
--
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]
