[GitHub] [kafka] jsancio commented on a change in pull request #10786: KAFKA-12787: Integrate controller snapshoting with raft client

[GitBox]

jsancio commented on a change in pull request #10786:
URL: https://github.com/apache/kafka/pull/10786#discussion_r649281362



##########
File path: 
metadata/src/main/java/org/apache/kafka/controller/QuorumController.java
##########
@@ -1009,7 +999,7 @@ private QuorumController(LogContext logContext,
             snapshotRegistry, sessionTimeoutNs, replicaPlacer);
         this.featureControl = new FeatureControlManager(supportedFeatures, 
snapshotRegistry);
         this.producerIdControlManager = new 
ProducerIdControlManager(clusterControl, snapshotRegistry);
-        this.snapshotGeneratorManager = new 
SnapshotGeneratorManager(snapshotWriterBuilder);
+        this.snapshotGeneratorManager = new 
SnapshotGeneratorManager(raftClient::createSnapshot);

Review comment:
       Fair enough. Removing the `BiFunction` from the constructor. 
`SnapshotGeneratorManager` is an inner class so it should have access to the 
`raftClient`.
   
   > Was this done for testing or something?
   
   I am not sure why this was added. It is not used on tests. I think the 
previous code didn't have access to the `raftClient` because this code was 
merged before reversing the dependency between the `metadata` project and the 
`raft` project.




-- 
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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org