vamossagar12 commented on a change in pull request #9756:
URL: https://github.com/apache/kafka/pull/9756#discussion_r556429212



##########
File path: 
raft/src/main/java/org/apache/kafka/raft/internals/BatchAccumulator.java
##########
@@ -37,6 +37,7 @@
     private final Time time;
     private final SimpleTimer lingerTimer;
     private final int lingerMs;
+    private final int minFlushSize;

Review comment:
       @hachikuji , @jsancio I agree to the points. I am slightly confused on 
the advantage of letting the config to be set higher than maxBatchSize(1MB) and 
then limiting the effective batch size to the minimum of `maxUnflushedBytes` 
and `maxBatchSize`. Won't it lead to confusion in terms of usage?
   Infact, I have followed a similar approach in the PR in a different way. 
What I have done is restricting the value of this new config within a range of 
0(which is wrong, need to change) to maxBatchSize - 1:
   
   
   ```            .define(QUORUM_FLUSH_MIN_SIZE_BYTES_CONFIG,
                   ConfigDef.Type.INT,
                   1024,
                   between(0, (1024 * 1024) - 1),
                   ConfigDef.Importance.MEDIUM,
                   QUORUM_FLUSH_MIN_SIZE_BYTES_DOC);
   ```
   
   I feel this way, what the config provides and what actually happens remains 
the same and effectively, this will be equivalent to `min(maxBatchsize, 
maxUnflushedBytes)`.
   
   Also @hachikuji , wanted to understand how the newly proposed config; 
`quorum.append.max.linger.ms` would interplay with the existing 
`quorum.append.linger.ms` config.  As per my understanding, the moment 
`quorum.append.linger.ms` is crossed, the flush would start. This happens even 
in this new implementation irrespective of hitting `maxUnflushedBytes` or not. 
Are you suggesting that we still hold onto writes until we hit the 
`quorum.append.max.linger.ms`  thereby overriding `quorum.append.linger.ms`? 
   
   Considering just these 2 configs, I am confused of how it will work but if 
we add the `maxUnflushedBytes`, then probably we will have to write some logic 
to hold onto writes between `quorum.append.linger.ms` and 
`quorum.append.max.linger.ms`. Maybe, we give further time between this time 
range to let the writes be accumulated. The moment we hit either of the 2 i.e 
`maxUnflushedBytes` or `quorum.append.max.linger.ms`, we flush the writes. This 
gives me the impression that we are giving more preference to 
`maxUnflushedBytes`. 
   
   Let me know your thoughts plz.
   




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


Reply via email to