Marton Greber has posted comments on this change. ( http://gerrit.cloudera.org:8080/24412 )
Change subject: KUDU-3429 Add extensive test for delta generation (3/n) ...................................................................... Patch Set 1: (4 comments) http://gerrit.cloudera.org:8080/#/c/24412/1/src/kudu/tablet/compaction-highmem-test.cc File src/kudu/tablet/compaction-highmem-test.cc: http://gerrit.cloudera.org:8080/#/c/24412/1/src/kudu/tablet/compaction-highmem-test.cc@161 PS1, Line 161: // The docstring says "kFlushRounds = 10 * size_factor" but the implementation at line 314 has `kFlushRounds = 2 * size_factor`. Which is intended? If the implementation is correct, the doc should be updated. http://gerrit.cloudera.org:8080/#/c/24412/1/src/kudu/tablet/compaction-highmem-test.cc@505 PS1, Line 505: tablet()->GetRowSetsForTests(&rowsets); `total_delta_bytes` is accumulated but never asserted on or logged. The comment above says "Assert that a meaningful amount of delta data was produced" and "we expect well over 40 MB", but no `ASSERT_GT(total_delta_bytes, ...)` exists. This is dead code - either add the assertion or remove the block entirely. http://gerrit.cloudera.org:8080/#/c/24412/1/src/kudu/tablet/compaction-highmem-test.cc@535 PS1, Line 535: // Disable compaction memory budgeting for this rowset compaction op. The comment says "Disable compaction memory budgeting" but the code sets the flag to `true` (i.e. **enables** it). The test's final assertion expects the "beyond hard memory limit" log message, which only appears when budgeting is enabled - so the code is doing the right thing, but the comment is misleading. Should read "Enable compaction memory budgeting" or similar. http://gerrit.cloudera.org:8080/#/c/24412/1/src/kudu/tablet/compaction-highmem-test.cc@539 PS1, Line 539: // Enable compaction memory budgeting for next tests. The restore comment "Enable compaction memory budgeting for next tests." is also misleading since it's restoring the original value (which was already `true` from SetUp). Consider "Restore original value." or removing the comment. -- To view, visit http://gerrit.cloudera.org:8080/24412 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I8c337dba87a9d1cf549f9b53813da8d50e86b6b4 Gerrit-Change-Number: 24412 Gerrit-PatchSet: 1 Gerrit-Owner: Ashwani Raina <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Comment-Date: Tue, 09 Jun 2026 14:27:43 +0000 Gerrit-HasComments: Yes
