Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/20816 )
Change subject: [compaction/test] Add tests to generate heavy rowset compaction ...................................................................... Patch Set 6: (12 comments) http://gerrit.cloudera.org:8080/#/c/20816/6/src/kudu/tablet/compaction-test.cc File src/kudu/tablet/compaction-test.cc: http://gerrit.cloudera.org:8080/#/c/20816/6/src/kudu/tablet/compaction-test.cc@142 PS6, Line 142: void Why not to make this method returning Status instead? That would also help to get rid of quite irregular for a test LOG(FATAL), replacing it with Status::InvalidArgument(). http://gerrit.cloudera.org:8080/#/c/20816/6/src/kudu/tablet/compaction-test.cc@153 PS6, Line 153: CHECK_OK here and below: why not to use ASSERT_OK here, similar to the lines above? http://gerrit.cloudera.org:8080/#/c/20816/6/src/kudu/tablet/compaction-test.cc@164 PS6, Line 164: InsertOrUpsertTestRows Shouldn't this be wrapped into NO_FATALS() to exit as soon as any assertion in InsertOrUpsertTestRows() triggers? http://gerrit.cloudera.org:8080/#/c/20816/6/src/kudu/tablet/compaction-test.cc@176 PS6, Line 176: InsertOrUpsertTestRows ditto: either wrap into NO_FATALS() or into ASSERT_OK() if changing the signature of InsertOrUpsertTestRows() to return Status. http://gerrit.cloudera.org:8080/#/c/20816/6/src/kudu/tablet/compaction-test.cc@605 PS6, Line 605: heavy sized What's 'heavy sized'? Does it simply mean 'large' or there is some other meaning here? Please either use plain language or clarify on special semantics, if any. http://gerrit.cloudera.org:8080/#/c/20816/6/src/kudu/tablet/compaction-test.cc@606 PS6, Line 606: generates 1MB generates 1 MB deltas ? http://gerrit.cloudera.org:8080/#/c/20816/6/src/kudu/tablet/compaction-test.cc@607 PS6, Line 607: as per test needs nit: drop this part http://gerrit.cloudera.org:8080/#/c/20816/6/src/kudu/tablet/compaction-test.cc@610 PS6, Line 610: delta_mem_factor It would be great to document the 'delta_mem_factor' parameter as well. http://gerrit.cloudera.org:8080/#/c/20816/6/src/kudu/tablet/compaction-test.cc@632 PS6, Line 632: TestRowSetCompactionProceedWithNoBudgetingConstraints Do you think this test should run for ASAN/TSAN builds as well, or it should be limited to DEBUG/RELEASE builds only? http://gerrit.cloudera.org:8080/#/c/20816/6/src/kudu/tablet/compaction-test.cc@637 PS6, Line 637: GenHighMemConsumptionDeltas(2, 10); Here and in another test below: to avoid flakiness due to the amount of available memory on a test node, consider setting FLAGS_memory_limit_hard_bytes to some reasonable value that fits the parameters of the test. http://gerrit.cloudera.org:8080/#/c/20816/6/src/kudu/tablet/compaction-test.cc@648 PS6, Line 648: sw.stop(); nit: start/stop at different scope levels looks a bit odd; maybe move sw.stop() out this scope or move in sw.start()? http://gerrit.cloudera.org:8080/#/c/20816/6/src/kudu/tablet/compaction-test.cc@663 PS6, Line 663: TEST_F(TestCompaction, TestRowSetCompactionSkipWithBudgetingConstraints) { These two scenarios looks almost identical, the only differences are mem/deltas factors and whether the log messages contain the pattern. Could this be parameterized/moved into a function to avoid duplicating the code? -- To view, visit http://gerrit.cloudera.org:8080/20816 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I1996558e71c49314c6acf12faf854c796548318c Gerrit-Change-Number: 20816 Gerrit-PatchSet: 6 Gerrit-Owner: Ashwani Raina <ara...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <ale...@apache.org> Gerrit-Reviewer: Attila Bukor <abu...@apache.org> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Fri, 05 Jan 2024 17:39:43 +0000 Gerrit-HasComments: Yes
