Skip to content

Conversation

josefk31
Copy link
Contributor

@josefk31 josefk31 commented Aug 29, 2025

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.

Reviewers: Chia-Ping Tsai [email protected]

…tchSizeBytes

The original name is confusing which could cause engineers to make a
mistake and confuse the size with some other unit like number of records.
@github-actions github-actions bot added triage PRs from the community kraft small Small PRs labels Aug 29, 2025
@github-actions github-actions bot removed the triage PRs from the community label Sep 1, 2025
Changed maxBatchSize to maxBatchSizeBytes in BatchAccumulator.
@chia7712
Copy link
Member

chia7712 commented Sep 7, 2025

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

@josefk31
Copy link
Contributor Author

josefk31 commented Sep 8, 2025

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

@chia7712
Copy link
Member

chia7712 commented Sep 9, 2025

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 😄

@chia7712 chia7712 merged commit 1debe64 into apache:trunk Sep 9, 2025
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants