[kudu-CR] [compaction/test] Add tests to generate heavy rowset compaction
Hello Tidy Bot, Alexey Serbin, Attila Bukor, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20816 to look at the new patch set (#8). Change subject: [compaction/test] Add tests to generate heavy rowset compaction .. [compaction/test] Add tests to generate heavy rowset compaction This change implements a better approach to test out high memory compaction case. There is an existing patch: https://gerrit.cloudera.org/#/c/19278/ that uses different approach which does not scale well because of excessive memory usage by client. Patch contains two tests: 1. TestRowSetCompactionProceedWithNoBudgetingConstraints (generates deltas with memory requirements under budget) 2. TestRowSetCompactionSkipWithBudgetingConstraints (generates deltas with memory requirements over budget) and a helper function: GenHighMemConsumptionDeltas Using this helper function, callers can generate deltas of different sizes as per test needs. The size_factor can be used to achieve that. Increasing size_factor by 1 adds 1 MB worth of deltas as far as rowset compaction memory consumption is concerned. Change-Id: I1996558e71c49314c6acf12faf854c796548318c --- M src/kudu/tablet/compaction-test.cc 1 file changed, 141 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/16/20816/8 -- 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: newpatchset Gerrit-Change-Id: I1996558e71c49314c6acf12faf854c796548318c Gerrit-Change-Number: 20816 Gerrit-PatchSet: 8 Gerrit-Owner: Ashwani Raina Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [compaction/test] Add tests to generate heavy rowset compaction
Ashwani Raina 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 7: (16 comments) http://gerrit.cloudera.org:8080/#/c/20816/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20816/6//COMMIT_MSG@23 PS6, Line 23: this h > nit: this helper function Done 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: > Why not to make this method returning Status instead? That would also help Done http://gerrit.cloudera.org:8080/#/c/20816/6/src/kudu/tablet/compaction-test.cc@153 PS6, Line 153: (type = > here and below: why not to use ASSERT_OK here, similar to the lines above? Done http://gerrit.cloudera.org:8080/#/c/20816/6/src/kudu/tablet/compaction-test.cc@163 PS6, Line 163: > shouldn't this be int64_t too? also in UpdateOriginalRowsNoFlush Done http://gerrit.cloudera.org:8080/#/c/20816/6/src/kudu/tablet/compaction-test.cc@164 PS6, Line 164: > Shouldn't this be wrapped into NO_FATALS() to exit as soon as any assertion Done http://gerrit.cloudera.org:8080/#/c/20816/6/src/kudu/tablet/compaction-test.cc@176 PS6, Line 176: UpdateOriginalRowsNoF > ditto: either wrap into NO_FATALS() or into ASSERT_OK() if changing the sig Done http://gerrit.cloudera.org:8080/#/c/20816/6/src/kudu/tablet/compaction-test.cc@605 PS6, Line 605: (Mutation** > What's 'heavy sized'? Does it simply mean 'large' or there is some other m Done http://gerrit.cloudera.org:8080/#/c/20816/6/src/kudu/tablet/compaction-test.cc@606 PS6, Line 606: AndDelete(Row > generates 1 MB deltas ? Done http://gerrit.cloudera.org:8080/#/c/20816/6/src/kudu/tablet/compaction-test.cc@607 PS6, Line 607: > nit: drop this part Done http://gerrit.cloudera.org:8080/#/c/20816/6/src/kudu/tablet/compaction-test.cc@608 PS6, Line 608: // Workload to generate large sized deltas for compaction. > The commit message says increasing the size_factor by 1 increases the delta Done http://gerrit.cloudera.org:8080/#/c/20816/6/src/kudu/tablet/compaction-test.cc@610 PS6, Line 610: > It would be great to document the 'delta_mem_factor' parameter as well. Not applicable anymore. Removed this argument. Callers will take care of setting the flags now. http://gerrit.cloudera.org:8080/#/c/20816/6/src/kudu/tablet/compaction-test.cc@632 PS6, Line 632: > Do you think this test should run for ASAN/TSAN builds as well, or it shoul Theoretically, it may not make much sense for these tests to run for TSAN builds because there is no race condition as such to be found. But it would not hurt to see how compaction does under large workloads and potential memory overhead introduced by TSAN. ASAN may sound more desired in this case as against TSAN because running into memory errors are of higher probability here. I am of the opinion of keeping it enabled for both though. It may involve some memory overhead and increase in execution time. This is already a slow test so that should not be a problem. http://gerrit.cloudera.org:8080/#/c/20816/6/src/kudu/tablet/compaction-test.cc@637 PS6, Line 637: // to make sure that when rowset compaction is invoked, it takes > Here and in another test below: to avoid flakiness due to the amount of ava The problem with setting FLAGS_memory_limit_hard_bytes in the test is that it remains set for the lifetime of the test binary in global variable g_hard_limit. If single set would do the trick for these two tests, it would have be re-visited if there is an additional test added here, in future, that works on hard limit. If I want to change the limit to different value in this test or a different one under this binary, I will have to explicitly set the global variable, which is not a clean approach. If there is a workaround for all this, we still need to take into account the current consumption of the process while setting hard limit which seems to be somewhat an overhead for a simple test. It makes more sense to just keep it under legitimate and acceptable limit (i.e. 20 MB) for this case where we want compaction to proceed. If a mere 20 MB memory is not available on a test node, maybe adjusting this value isn't our big problem. Anyway, I will decrease it further to 2 MB. For second test below, the memory requirement is set for ~2 TB which is quite high for an average node. http://gerrit.cloudera.org:8080/#/c/20816/6/src/kudu/tablet/compaction-test.cc@648 PS6, Line 648: his test a > nit: start/stop at different scope levels looks a bit odd; maybe move sw.st Done http://gerrit.cloudera.org:8080/#/c/20816/6/src/kudu/tablet/compaction-test.cc@663 PS6, Line 663: // Create a memrowset with 10 rows and several updates. > These two scenarios looks almost
[kudu-CR] [compaction/test] Add tests to generate heavy rowset compaction
Hello Tidy Bot, Alexey Serbin, Attila Bukor, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20816 to look at the new patch set (#7). Change subject: [compaction/test] Add tests to generate heavy rowset compaction .. [compaction/test] Add tests to generate heavy rowset compaction This change implements a better approach to test out high memory compaction case. There is an existing patch: https://gerrit.cloudera.org/#/c/19278/ that uses different approach which does not scale well because of excessive memory usage by client. Patch contains two tests: 1. TestRowSetCompactionProceedWithNoBudgetingConstraints (generates deltas with memory requirements under budget) 2. TestRowSetCompactionSkipWithBudgetingConstraints (generates deltas with memory requirements over budget) and a helper function: GenHighMemConsumptionDeltas Using this helper function, callers can generate deltas of different sizes as per test needs. The size_factor can be used to achieve that. Increasing size_factor by 1 adds 1 MB worth of deltas as far as rowset compaction memory consumption is concerned. Change-Id: I1996558e71c49314c6acf12faf854c796548318c --- M src/kudu/tablet/compaction-test.cc 1 file changed, 142 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/16/20816/7 -- 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: newpatchset Gerrit-Change-Id: I1996558e71c49314c6acf12faf854c796548318c Gerrit-Change-Number: 20816 Gerrit-PatchSet: 7 Gerrit-Owner: Ashwani Raina Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [compaction/test] Add tests to generate heavy rowset compaction
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
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Attila Bukor
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Comment-Date: Fri, 05 Jan 2024 17:39:43 +
Gerrit-HasComments: Yes
[kudu-CR] [compaction/test] Add tests to generate heavy rowset compaction
Attila Bukor 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: (4 comments) http://gerrit.cloudera.org:8080/#/c/20816/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20816/6//COMMIT_MSG@23 PS6, Line 23: helper nit: this helper function 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@163 PS6, Line 163: int shouldn't this be int64_t too? also in UpdateOriginalRowsNoFlush http://gerrit.cloudera.org:8080/#/c/20816/6/src/kudu/tablet/compaction-test.cc@608 PS6, Line 608: // For example, to generate 5MB, set size_factor as 5. The commit message says increasing the size_factor by 1 increases the delta size by 10MB.. Can you fix this discrepancy? http://gerrit.cloudera.org:8080/#/c/20816/6/src/kudu/tablet/compaction-test.cc@911 PS6, Line 911: FLAGS_rowset_compaction_memory_estimate_enabled = true; It might be just me, but I think setting these flags here seems like a side-effect that I wouldn't necessary expect when calling this method. How about setting them in either the setup for these new tests or in the test cases themselves? Maybe with a helper method. -- 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 Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Comment-Date: Fri, 05 Jan 2024 16:15:23 + Gerrit-HasComments: Yes
[kudu-CR] [compaction/test] Add tests to generate heavy rowset compaction
Hello Tidy Bot, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20816 to look at the new patch set (#6). Change subject: [compaction/test] Add tests to generate heavy rowset compaction .. [compaction/test] Add tests to generate heavy rowset compaction This change implements a better approach to test out high memory compaction case. There is an existing patch: https://gerrit.cloudera.org/#/c/19278/ that uses different approach which does not scale well because of excessive memory usage by client. Patch contains two tests: 1. TestRowSetCompactionProceedWithNoBudgetingConstraints (generates deltas with memory requirements under budget) 2. TestRowSetCompactionSkipWithBudgetingConstraints (generates deltas with memory requirements over budget) and a helper function: GenHighMemConsumptionDeltas Using helper function, callers can generate deltas of different sizes as per test needs. The size_factor can be used to achieve that. Increasing size_factor by 1 adds 10MB worth of deltas as far as rowset compaction memory consumption is concerned. Change-Id: I1996558e71c49314c6acf12faf854c796548318c --- M src/kudu/tablet/compaction-test.cc 1 file changed, 142 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/16/20816/6 -- 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: newpatchset Gerrit-Change-Id: I1996558e71c49314c6acf12faf854c796548318c Gerrit-Change-Number: 20816 Gerrit-PatchSet: 6 Gerrit-Owner: Ashwani Raina Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [compaction/test] Add tests to generate heavy rowset compaction
Hello Tidy Bot, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20816 to look at the new patch set (#5). Change subject: [compaction/test] Add tests to generate heavy rowset compaction .. [compaction/test] Add tests to generate heavy rowset compaction This change implements a better approach to test out high memory compaction case. There is an existing patch: https://gerrit.cloudera.org/#/c/19278/ that uses different approach which does not scale well because of excessive memory usage by client. Patch contains two tests: 1. TestRowSetCompactionProceedWithNoBudgetingConstraints (generates deltas with memory requirements under budget) 2. TestRowSetCompactionSkipWithBudgetingConstraints (generates deltas with memory requirements over budget) and a helper function: GenHighMemConsumptionDeltas Using helper function, callers can generate deltas of different sizes as per test needs. The size_factor can be used to achieve that. Increasing size_factor by 1 adds 10MB worth of deltas as far as rowset compaction memory consumption is concerned. Change-Id: I1996558e71c49314c6acf12faf854c796548318c --- M src/kudu/tablet/compaction-test.cc 1 file changed, 142 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/16/20816/5 -- 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: newpatchset Gerrit-Change-Id: I1996558e71c49314c6acf12faf854c796548318c Gerrit-Change-Number: 20816 Gerrit-PatchSet: 5 Gerrit-Owner: Ashwani Raina Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [compaction/test] Add tests to generate heavy rowset compaction
Hello Tidy Bot, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20816 to look at the new patch set (#4). Change subject: [compaction/test] Add tests to generate heavy rowset compaction .. [compaction/test] Add tests to generate heavy rowset compaction This change implements a better approach to test out high memory compaction case. There is an existing patch: https://gerrit.cloudera.org/#/c/19278/ that uses different approach which does not scale well because of excessive memory usage by client. Patch contains two tests: 1. TestRowSetCompactionProceedWithNoBudgetingConstraints (generates deltas with memory requirements under budget) 2. TestRowSetCompactionSkipWithBudgetingConstraints (generates deltas with memory requirements over budget) and a helper function: GenHighMemConsumptionDeltas Using helper function, callers can generate deltas of different sizes as per test needs. The size_factor can be used to achieve that. Increasing size_factor by 1 adds 10MB worth of deltas as far as rowset compaction memory consumption is concerned. Change-Id: I1996558e71c49314c6acf12faf854c796548318c --- M src/kudu/tablet/compaction-test.cc 1 file changed, 169 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/16/20816/4 -- 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: newpatchset Gerrit-Change-Id: I1996558e71c49314c6acf12faf854c796548318c Gerrit-Change-Number: 20816 Gerrit-PatchSet: 4 Gerrit-Owner: Ashwani Raina Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241)
[kudu-CR] [compaction/test] Add tests to generate heavy rowset compaction
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20816 to look at the new patch set (#3). Change subject: [compaction/test] Add tests to generate heavy rowset compaction .. [compaction/test] Add tests to generate heavy rowset compaction This change implements a better approach to test out high memory compaction case. There is an existing patch: https://gerrit.cloudera.org/#/c/19278/ that uses different approach which does not scale well because of excessive memory usage by client. Patch contains two tests: 1. TestRowSetCompactionProceedWithNoBudgetingConstraints (generates deltas with memory requirements under budget) 2. TestRowSetCompactionSkipWithBudgetingConstraints (generates deltas with memory requirements over budget) and a helper function: GenHighMemConsumptionDeltas Using helper function, callers can generate deltas of different sizes as per test needs. The size_factor can be used to achieve that. Increasing size_factor by 1 adds 10MB worth of deltas as far as rowset compaction memory consumption is concerned. Change-Id: I1996558e71c49314c6acf12faf854c796548318c --- M src/kudu/tablet/compaction-test.cc 1 file changed, 169 insertions(+), 0 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/16/20816/3 -- 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: newpatchset Gerrit-Change-Id: I1996558e71c49314c6acf12faf854c796548318c Gerrit-Change-Number: 20816 Gerrit-PatchSet: 3 Gerrit-Owner: Ashwani Raina Gerrit-Reviewer: Kudu Jenkins (120)
