You are viewing a plain text version of this content. The canonical link for it is here.
Posted to jira@kafka.apache.org by "josefk31 (via GitHub)" <gi...@apache.org> on 2025/08/29 19:51:12 UTC

[PR] MINOR Rename RecordSnapshotWriter.Builder.setMaxBatchSize to setMaxBatchSizeBytes [kafka]

josefk31 opened a new pull request, #20446:
URL: https://github.com/apache/kafka/pull/20446

   The original name is confusing which could cause engineers to make a mistake and confuse the `batchSize` with some other unit like number of records.
   


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

Re: [PR] MINOR Rename RecordSnapshotWriter.Builder.setMaxBatchSize to setMaxBatchSizeBytes [kafka]

Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on PR #20446:
URL: https://github.com/apache/kafka/pull/20446#issuecomment-3268892847

   > If that is the case, on the one hand this is just a refactoring; on the other hand it's not an area of the code I'm familiar with. Not a huge deal to change - it seems like other usages are mostly local variables. IG it can't hurt. What do you think?
   
   It's not a big deal, so it's fine to leave it as it is :smile: 


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

Re: [PR] MINOR Rename RecordSnapshotWriter.Builder.setMaxBatchSize to setMaxBatchSizeBytes [kafka]

Posted by "josefk31 (via GitHub)" <gi...@apache.org>.
josefk31 commented on PR #20446:
URL: https://github.com/apache/kafka/pull/20446#issuecomment-3266994013

   > It seems the word `maxBatchSize` is still used in code case. Should we align them in this PR?
   
   @chia7712 do you mean it's still used [here](https://github.com/apache/kafka/blob/f7593db2878c7332ec8c37ef78ccd0bc704e4fcc/core/src/main/scala/kafka/coordinator/transaction/TransactionStateManager.scala#L161) (and a few other places)?
   
   If that is the case, on the one hand this is just a refactoring; on the other hand it's not an area of the code I'm familiar with. Not a huge deal to change - it seems like other usages are mostly local variables. IG it can't hurt. What do you think? 


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

Re: [PR] MINOR Rename RecordSnapshotWriter.Builder.setMaxBatchSize to setMaxBatchSizeBytes [kafka]

Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on PR #20446:
URL: https://github.com/apache/kafka/pull/20446#issuecomment-3264160374

   It seems the word `maxBatchSize` is still used in code case. Should we align them in this PR?


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

Re: [PR] MINOR Rename RecordSnapshotWriter.Builder.setMaxBatchSize to setMaxBatchSizeBytes [kafka]

Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 commented on code in PR #20446:
URL: https://github.com/apache/kafka/pull/20446#discussion_r2312230645


##########
raft/src/main/java/org/apache/kafka/snapshot/RecordsSnapshotWriter.java:
##########
@@ -57,7 +57,7 @@ private RecordsSnapshotWriter(
             snapshot.snapshotId().epoch(),
             0,
             Integer.MAX_VALUE,
-            maxBatchSize,
+            maxBatchSizeBytes,

Review Comment:
   Could you please apply the same changes to `BatchAccumulator`?



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

Re: [PR] MINOR Rename RecordSnapshotWriter.Builder.setMaxBatchSize to setMaxBatchSizeBytes [kafka]

Posted by "josefk31 (via GitHub)" <gi...@apache.org>.
josefk31 commented on code in PR #20446:
URL: https://github.com/apache/kafka/pull/20446#discussion_r2325454643


##########
raft/src/main/java/org/apache/kafka/snapshot/RecordsSnapshotWriter.java:
##########
@@ -57,7 +57,7 @@ private RecordsSnapshotWriter(
             snapshot.snapshotId().epoch(),
             0,
             Integer.MAX_VALUE,
-            maxBatchSize,
+            maxBatchSizeBytes,

Review Comment:
   Thanks for the comment! I've updated `BatchAccumulator`



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

Re: [PR] MINOR Rename RecordSnapshotWriter.Builder.setMaxBatchSize to setMaxBatchSizeBytes [kafka]

Posted by "chia7712 (via GitHub)" <gi...@apache.org>.
chia7712 merged PR #20446:
URL: https://github.com/apache/kafka/pull/20446


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