showuon commented on code in PR #14289: URL: https://github.com/apache/kafka/pull/14289#discussion_r1493974832
########## core/src/main/scala/kafka/server/ReplicaManager.scala: ########## @@ -1408,6 +1408,16 @@ class ReplicaManager(val config: KafkaConfig, // progress in such cases and don't need to report a `RecordTooLargeException` new FetchDataInfo(givenFetchedDataInfo.fetchOffsetMetadata, MemoryRecords.EMPTY) } else { + // For last entries we assume that it is hot enough to still have all data in page cache. + // Most of fetch requests are fetching from the tail of the log, so this optimization should save + // call of additional sendfile(2) targeting /dev/null for populating page cache significantly. + if (!givenFetchedDataInfo.isLastSegment && givenFetchedDataInfo.records.isInstanceOf[FileRecords]) { + try { + givenFetchedDataInfo.records.asInstanceOf[FileRecords].prepareForRead() + } catch { + case e: Exception => warn("failed to prepare cache for read", e) Review Comment: I'd argue if we need to log as WARN here since if this there's something wrong with the prepareForRead, the WARN logs will keep happening, but it won't impact the fetch at all, just have possible performance impact. Could we use INFO or maybe DEBUG level here? Also, add more info in the log message, maybe: `Failed to prepare cache for read for performance improvement. This can be ignored if the fetch behavior works without any issue.` WDYT? -- 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-unsubscr...@kafka.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org