josefk31 commented on code in PR #21028:
URL: https://github.com/apache/kafka/pull/21028#discussion_r2942785315
##########
raft/src/main/java/org/apache/kafka/raft/internals/KRaftControlRecordStateMachine.java:
##########
@@ -233,7 +234,11 @@ private void checkOffsetIsValid(long offset) {
private void maybeLoadLog() {
while (log.endOffset().offset() > nextOffset) {
- LogFetchInfo info = log.read(nextOffset, Isolation.UNCOMMITTED);
+ LogFetchInfo info = log.read(
+ nextOffset,
+ Isolation.UNCOMMITTED,
+ KafkaRaftClient.MAX_FETCH_SIZE_BYTES
Review Comment:
Could you expand why you have a preference for this?
~In my mind, it makes sense to leave this unchanged because we're bounding
the amount of memory-space that we can allocate in a single read(xxxx) call.
After each iteration of the inner loop, the records we have loaded into memory
have the potential to be deallocated by GC. But ofc this assumes a more
simplistic model for GC and that it can actually free the records but in
principal the code loads 8MiB, processes the records and no longer needs them
seems reasonable to me. But having no maximum means that the amount of space
allocated depends on the size of log on disk. That leads to unpredictable
memory usage. We could end up in a situation where there is not enough memory
to allocate the entire thing.~
EDIT: Seems like `FileRecord` actually doesn't allocate anything to memory,
it seems to just remember the position being read from the file handle...
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
