[GitHub] [kafka] showuon commented on a change in pull request #10969: KAFKA-10884: Limit the size of Fetch and FetchSnapshot response based on broker configuration

[GitBox <gi...@apache.org>]

showuon commented on a change in pull request #10969:
URL: https://github.com/apache/kafka/pull/10969#discussion_r663730489



##########
File path: raft/src/main/java/org/apache/kafka/raft/RaftConfig.java
##########
@@ -138,14 +139,15 @@ private UnknownAddressSpec() {
         }
     }
 
-    public RaftConfig(AbstractConfig abstractConfig) {
+    public RaftConfig(AbstractConfig abstractConfig, int replicaFetchResponseMaxBytes) {
         this(parseVoterConnections(abstractConfig.getList(QUORUM_VOTERS_CONFIG)),
             abstractConfig.getInt(QUORUM_REQUEST_TIMEOUT_MS_CONFIG),
             abstractConfig.getInt(QUORUM_RETRY_BACKOFF_MS_CONFIG),
             abstractConfig.getInt(QUORUM_ELECTION_TIMEOUT_MS_CONFIG),
             abstractConfig.getInt(QUORUM_ELECTION_BACKOFF_MAX_MS_CONFIG),
             abstractConfig.getInt(QUORUM_FETCH_TIMEOUT_MS_CONFIG),
-            abstractConfig.getInt(QUORUM_LINGER_MS_CONFIG));
+            abstractConfig.getInt(QUORUM_LINGER_MS_CONFIG),
+            replicaFetchResponseMaxBytes);

Review comment:
       Can't we get the `replicaFetchResponseMaxBytes` via passed in `abstractConfig`?

##########
File path: raft/src/test/java/org/apache/kafka/raft/KafkaRaftClientSnapshotTest.java
##########
@@ -48,6 +48,9 @@
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
 final public class KafkaRaftClientSnapshotTest {
+
+    private static final int REPLICA_FETCH_RESPONSE_MAX_BYTES = 8 * 1024 * 1024;

Review comment:
       Why can't we use `KafkaRaftClient.MAX_BATCH_SIZE_BYTES` here?

##########
File path: core/src/test/scala/unit/kafka/server/KafkaConfigTest.scala
##########
@@ -1054,7 +1054,8 @@ class KafkaConfigTest {
   private def assertValidQuorumVoters(value: String, expectedVoters: util.Map[Integer, AddressSpec]): Unit = {
     val props = TestUtils.createBrokerConfig(0, TestUtils.MockZkConnect)
     props.put(RaftConfig.QUORUM_VOTERS_CONFIG, value)
-    val raftConfig = new RaftConfig(KafkaConfig.fromProps(props))
+    val kafkaConfig = KafkaConfig.fromProps(props)
+    val raftConfig = new RaftConfig(kafkaConfig, kafkaConfig.replicaFetchResponseMaxBytes)

Review comment:
       Why should we put an additional parameters into `RaftConfig` here? The `replicaFetchResponseMaxBytes` is already in `kafkaConfig`, isn't it?




-- 
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: jira-unsubscribe@kafka.apache.org

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