Alexey Serbin 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 2: (8 comments) http://gerrit.cloudera.org:8080/#/c/24412/2//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/24412/2//COMMIT_MSG@12 PS2, Line 12: eligibile eligible http://gerrit.cloudera.org:8080/#/c/24412/2//COMMIT_MSG@17 PS2, Line 17: * Use pseudo-random byte pattern to minimize the effect of compression. : If there are repeated characters, compression would bring memory : requirements of a CFile block memory to a minimum, keeping RSS flat : during compaction. I'm not sure I understand this logic. If there is some compression, then I'd expect on-disk data to be more compact, but it would inflate in size when reading it into the memory while operating with the loaded data, right? In this sense, having compressed data on disk would require much more RSS during merge compaction runtime than non-compressed data of the same size on the disk. And here we want as much memory size "explosion" as possible, so highly compressed data would be exactly what we'd want for such a test, no? http://gerrit.cloudera.org:8080/#/c/24412/2//COMMIT_MSG@21 PS2, Line 21: defacto nit: usually it's spelled as two words, i.e. 'de facto' http://gerrit.cloudera.org:8080/#/c/24412/2/src/kudu/tablet/compaction-highmem-test.cc File src/kudu/tablet/compaction-highmem-test.cc: http://gerrit.cloudera.org:8080/#/c/24412/2/src/kudu/tablet/compaction-highmem-test.cc@77 PS2, Line 77: // Wide column used by GenHighMemConsumptionDeltasWithFrequentFlush to make : // each delta mutation ~1 KB on disk. This lets the test reach a large total : // delta volume with fewer tablet operations in a short span of time. : // Existing tests leave this column NULL so their behaviour is unchanged. : CHECK_OK(builder.AddNullableColumn("large_val", STRING)); Did you explore specifying some custom/non-default type of encodings for this string column and other columns of this table that could make the deltas larger when loading the data into memory during rutime? http://gerrit.cloudera.org:8080/#/c/24412/2/src/kudu/tablet/compaction-highmem-test.cc@471 PS2, Line 471: … Here and elsewhere in this file: please avoid using non-ASCII characters in the code unless it's necessary for functionality. http://gerrit.cloudera.org:8080/#/c/24412/2/src/kudu/tablet/compaction-highmem-test.cc@498 PS2, Line 498: TestHighDeltaVolumeWithFrequentFlushForRowsetCompaction nit: there isn't much need in adding 'Test' into the scenario name, it's already present in the test name -- TestHighMemCompaction http://gerrit.cloudera.org:8080/#/c/24412/2/src/kudu/tablet/compaction-highmem-test.cc@501 PS2, Line 501: 30-40 seconds How long does it take to run with TSAN instrumentation? Should we skip this test at all in TSAN builds? http://gerrit.cloudera.org:8080/#/c/24412/2/src/kudu/tablet/compaction-highmem-test.cc@546 PS2, Line 546: ASSERT_STR_MATCHES( : JoinStrings(sink.logged_msgs(), "\n"), : "beyond hard memory limit of.*Rowset merge compaction ops consumption:"); Since the hard memory limit is set deterministically to 1GiB, is there any certain threshold of how much this should be going over the 1GiB limit when everything goes as planned? If yes, does it makes sense to assert on this here? -- 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: 2 Gerrit-Owner: Ashwani Raina <[email protected]> Gerrit-Reviewer: Abhishek Chennaka <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Ashwani Raina <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Marton Greber <[email protected]> Gerrit-Reviewer: Zoltan Martonka <[email protected]> Gerrit-Comment-Date: Tue, 23 Jun 2026 22:22:22 +0000 Gerrit-HasComments: Yes
