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

Reply via email to