ibessonov commented on code in PR #4005:
URL: https://github.com/apache/ignite-3/pull/4005#discussion_r1711323048
##########
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:
We already have `RaftServiceFactory` here, it's used for instantiating log
storage factory, I believe. Please provide an explanation of why we couldn't
extend this factory, and had to add another abstraction.
This configurator looks a little broken, for two reasons:
- It allows for arbitrary configuration modification. It might be flexible,
I know, but it's also prone to abuse.
- It accepts `Object` as a parameter. In order to provide a meaningful
implementation you must have an implemetation module in classpath, not only
`raft-api`. This makes `raft-api` completely redundant, by making it implicitly
dependent from the implementation module.
Was there any other way?
##########
modules/raft/src/integrationTest/java/org/apache/ignite/internal/raft/ItLearnersTest.java:
##########
@@ -89,9 +93,9 @@ private static TestWriteCommand createWriteCommand(String
value) {
}
private static final List<NetworkAddress> ADDRS = List.of(
- new NetworkAddress("localhost", 5000),
new NetworkAddress("localhost", 5001),
- new NetworkAddress("localhost", 5002)
+ new NetworkAddress("localhost", 5002),
+ new NetworkAddress("localhost", 5003)
Review Comment:
Excuse me?
--
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]
