[kudu-CR] [compaction] Add memory estimation unit test
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/20787 ) Change subject: [compaction] Add memory estimation unit test .. Patch Set 6: (1 comment) http://gerrit.cloudera.org:8080/#/c/20787/6/src/kudu/tablet/compaction_policy-test.cc File src/kudu/tablet/compaction_policy-test.cc: http://gerrit.cloudera.org:8080/#/c/20787/6/src/kudu/tablet/compaction_policy-test.cc@651 PS6, Line 651: *1024*1024 > style nit for here and below: separate operation symbol from the operands b One more point that Ashwani pointed to. With trying to set FLAGS_memory_limit_hard_bytes, there are two issues: * Once it's set, the hard memory limit is modified for all the tests in this binary, and the setting isn't changing back even if rolling back the flag's setting. That's might lead to very unexpected outcomes, especially if randomizing the set of scenarios to run (i.e. when using `--gtest_shuffle`). * From the other side, is this setting at line 651 actually effective (i.e. does it set the hard memory limit as expected)? Did you check that InitLimits() hasn't been called earlier when running this test scenario? -- To view, visit http://gerrit.cloudera.org:8080/20787 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5397c7e7bf29942a610b0b8203b6918811c49bbb Gerrit-Change-Number: 20787 Gerrit-PatchSet: 6 Gerrit-Owner: Ádám Bakai Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Ádám Bakai Gerrit-Comment-Date: Tue, 30 Jan 2024 06:16:15 + Gerrit-HasComments: Yes
[kudu-CR] [compaction] Add memory estimation unit test
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/20787 ) Change subject: [compaction] Add memory estimation unit test .. Patch Set 6: (1 comment) http://gerrit.cloudera.org:8080/#/c/20787/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20787/6//COMMIT_MSG@9 PS6, Line 9: FLAGS_rowset_compaction_memory_estimate_enabled was always set to : false during testing, so this part of the code was not executed in : testing environment > Hi Alexey, Adam, Cool! If you want to get feedback from particular people for a certain patch, feel free to add them as reviewers for the patch. -- To view, visit http://gerrit.cloudera.org:8080/20787 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5397c7e7bf29942a610b0b8203b6918811c49bbb Gerrit-Change-Number: 20787 Gerrit-PatchSet: 6 Gerrit-Owner: Ádám Bakai Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Ádám Bakai Gerrit-Comment-Date: Fri, 05 Jan 2024 17:00:22 + Gerrit-HasComments: Yes
[kudu-CR] [compaction] Add memory estimation unit test
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/20787 ) Change subject: [compaction] Add memory estimation unit test .. Patch Set 6: (1 comment) http://gerrit.cloudera.org:8080/#/c/20787/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/20787/6//COMMIT_MSG@9 PS6, Line 9: FLAGS_rowset_compaction_memory_estimate_enabled was always set to : false during testing, so this part of the code was not executed in : testing environment > Indeed! I recall that when I was working on 1556a353e, I verified the code Hi Alexey, Adam, Just a side note - while the test added here does go through the memory budgeting estimation code added in 1556a353e, it relies on MockDiskRowSet and its size that is set by the test to ensure it is more than the budget. That does the trick and test out the budgeting checks with mock rowsets. Additionally, I have added unit tests to test out the same budgeting checks with actual workload that generates deltas with specified memory requirements for compaction. You can check it out here: https://gerrit.cloudera.org/#/c/20816/ -- To view, visit http://gerrit.cloudera.org:8080/20787 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5397c7e7bf29942a610b0b8203b6918811c49bbb Gerrit-Change-Number: 20787 Gerrit-PatchSet: 6 Gerrit-Owner: Ádám Bakai Gerrit-Reviewer: Abhishek Chennaka Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Ádám Bakai Gerrit-Comment-Date: Wed, 03 Jan 2024 06:23:50 + Gerrit-HasComments: Yes
[kudu-CR] [compaction] Add memory estimation unit test
Alexey Serbin has posted comments on this change. (
http://gerrit.cloudera.org:8080/20787 )
Change subject: [compaction] Add memory estimation unit test
..
Patch Set 6:
(10 comments)
http://gerrit.cloudera.org:8080/#/c/20787/6//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/20787/6//COMMIT_MSG@9
PS6, Line 9: FLAGS_rowset_compaction_memory_estimate_enabled was always set to
: false during testing, so this part of the code was not executed in
: testing environment
Indeed! I recall that when I was working on 1556a353e, I verified the code
worked with the data captured at a real system, but then I cleaned up the units
for the newly introduced flags, and screwed up, forgetting to update the factor
as needed. Ashwani fixed the issue with ae7b08c00, but it's great to
eventually have a test for this.
Thank you very much for adding the test!
http://gerrit.cloudera.org:8080/#/c/20787/6/src/kudu/tablet/compaction_policy-test.cc
File src/kudu/tablet/compaction_policy-test.cc:
http://gerrit.cloudera.org:8080/#/c/20787/6/src/kudu/tablet/compaction_policy-test.cc@648
PS6, Line 648: TestMemoryEstimation
nit: 'Test' is already a part of the name of the test itself
('TestCompactionPolicy'); consider not duplicating it in the name of the
scenario, i.e. renaming this into 'MemoryEstimation'
http://gerrit.cloudera.org:8080/#/c/20787/6/src/kudu/tablet/compaction_policy-test.cc@651
PS6, Line 651: 2
Why is this magic '2' factor? Is it possible to get rid of it, correspondingly
updating the sizes of mock rowsets below?
http://gerrit.cloudera.org:8080/#/c/20787/6/src/kudu/tablet/compaction_policy-test.cc@651
PS6, Line 651: *1024*1024
style nit for here and below: separate operation symbol from the operands by
space
http://gerrit.cloudera.org:8080/#/c/20787/6/src/kudu/tablet/compaction_policy-test.cc@658
PS6, Line 658: 20
Is it crucial to have this factor 20? Isn't the compaction size estimation
good enough to work as expected with a couple of rowsets each of
memory_limit_hard_bytes size?
http://gerrit.cloudera.org:8080/#/c/20787/6/src/kudu/tablet/compaction_policy-test.cc@663
PS6, Line 663: picked.size(), 2
nit for here and below: the expected size comes first, otherwise it's harder to
decipher the error message when the assertion triggers
http://gerrit.cloudera.org:8080/#/c/20787/6/src/kudu/tablet/compaction_policy-test.cc@672
PS6, Line 672: for (const auto& str : post_capture_logs.logged_msgs()) {
: sum += str;
: }
: ASSERT_STR_CONTAINS(sum, "removed from compaction input due
to memory constraints");
Consider using JoinStrings() from strings/join.h
Isn't it enough just to search for the pattern in each logged message? I'd
think there isn't any concurrency involved here, so it would work, no?
http://gerrit.cloudera.org:8080/#/c/20787/6/src/kudu/tablet/diskrowset.cc
File src/kudu/tablet/diskrowset.cc:
http://gerrit.cloudera.org:8080/#/c/20787/6/src/kudu/tablet/diskrowset.cc@794
PS6, Line 794: uint64_t DiskRowSet::OnDiskDeltaSize() const {
Once this new method is introduced, why not to update the code in
Tablet::DoMergeCompactionOrFlush(), removing the down-casting?
http://gerrit.cloudera.org:8080/#/c/20787/6/src/kudu/tablet/rowset.cc
File src/kudu/tablet/rowset.cc:
http://gerrit.cloudera.org:8080/#/c/20787/6/src/kudu/tablet/rowset.cc@266
PS6, Line 266: const shared_ptr &rs
style nit: the ampersand sticks to the type, not the variable
Also, consider using 'const auto&' instead.
http://gerrit.cloudera.org:8080/#/c/20787/6/src/kudu/tablet/rowset.cc@266
PS6, Line 266: new_rowsets_
What about 'old_rowsets_'? Those also consume some disk space, no?
--
To view, visit http://gerrit.cloudera.org:8080/20787
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5397c7e7bf29942a610b0b8203b6918811c49bbb
Gerrit-Change-Number: 20787
Gerrit-PatchSet: 6
Gerrit-Owner: Ádám Bakai
Gerrit-Reviewer: Abhishek Chennaka
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Ashwani Raina
Gerrit-Reviewer: Attila Bukor
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Ádám Bakai
Gerrit-Comment-Date: Tue, 02 Jan 2024 21:23:08 +
Gerrit-HasComments: Yes
[kudu-CR] [compaction] Add memory estimation unit test
Hello Alexey Serbin, Ashwani Raina, Attila Bukor, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20787 to look at the new patch set (#6). Change subject: [compaction] Add memory estimation unit test .. [compaction] Add memory estimation unit test FLAGS_rowset_compaction_memory_estimate_enabled was always set to false during testing, so this part of the code was not executed in testing environment. Since it wasn't executed, it needed some changes so that it can run in testing,too: 1. MockDiskRowSet can not be downcasted to DiskRowSet, so a new function called OnDiskDeltaSize was introduced into RowSet, so now downcasting is not needed anymore and the OnDiskDeltaSize function is used instead. 2. metrics_ is not set up during testing, so it is skipped if it is not initialized. Change-Id: I5397c7e7bf29942a610b0b8203b6918811c49bbb --- M src/kudu/tablet/compaction_policy-test.cc M src/kudu/tablet/compaction_policy.cc M src/kudu/tablet/diskrowset.cc M src/kudu/tablet/diskrowset.h M src/kudu/tablet/memrowset.h M src/kudu/tablet/mock-rowsets.h M src/kudu/tablet/rowset.cc M src/kudu/tablet/rowset.h 8 files changed, 87 insertions(+), 11 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/87/20787/6 -- To view, visit http://gerrit.cloudera.org:8080/20787 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I5397c7e7bf29942a610b0b8203b6918811c49bbb Gerrit-Change-Number: 20787 Gerrit-PatchSet: 6 Gerrit-Owner: Ádám Bakai Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Ádám Bakai
[kudu-CR] [compaction] Add memory estimation unit test
Ádám Bakai has posted comments on this change. ( http://gerrit.cloudera.org:8080/20787 ) Change subject: [compaction] Add memory estimation unit test .. Patch Set 1: (3 comments) http://gerrit.cloudera.org:8080/#/c/20787/1/src/kudu/tablet/compaction_policy-test.cc File src/kudu/tablet/compaction_policy-test.cc: http://gerrit.cloudera.org:8080/#/c/20787/1/src/kudu/tablet/compaction_policy-test.cc@647 PS1, Line 647: TestMemoryEstimation > You could also add an ASSERT_STR_CONTAINS/ASSERT_STR_NOT_CONTAINS at the en Done http://gerrit.cloudera.org:8080/#/c/20787/1/src/kudu/tablet/compaction_policy-test.cc@649 PS1, Line 649: FLAGS_rowset_compaction_memory_estimate_enabled > Never mind! That should be ok. Done http://gerrit.cloudera.org:8080/#/c/20787/5/src/kudu/tablet/compaction_policy.cc File src/kudu/tablet/compaction_policy.cc: http://gerrit.cloudera.org:8080/#/c/20787/5/src/kudu/tablet/compaction_policy.cc@174 PS5, Line 174: > Could you also please take care of this? Done -- To view, visit http://gerrit.cloudera.org:8080/20787 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5397c7e7bf29942a610b0b8203b6918811c49bbb Gerrit-Change-Number: 20787 Gerrit-PatchSet: 1 Gerrit-Owner: Ádám Bakai Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Ádám Bakai Gerrit-Comment-Date: Tue, 19 Dec 2023 12:13:15 + Gerrit-HasComments: Yes
[kudu-CR] [compaction] Add memory estimation unit test
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/20787 ) Change subject: [compaction] Add memory estimation unit test .. Patch Set 5: Btw, I have a patch going out for review, shortly, that does something similar by adding two unit tests to exercise the rowset compaction. The difference is that here we are setting delta size programatically along with other parameters. The patch I am working on has two unit tests with and without budget restrictions and actual creation of deltas using update workload. Feel free to take a look and provide comments. -- To view, visit http://gerrit.cloudera.org:8080/20787 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5397c7e7bf29942a610b0b8203b6918811c49bbb Gerrit-Change-Number: 20787 Gerrit-PatchSet: 5 Gerrit-Owner: Ádám Bakai Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Ádám Bakai Gerrit-Comment-Date: Tue, 19 Dec 2023 12:08:02 + Gerrit-HasComments: No
[kudu-CR] [compaction] Add memory estimation unit test
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/20787 ) Change subject: [compaction] Add memory estimation unit test .. Patch Set 5: (1 comment) http://gerrit.cloudera.org:8080/#/c/20787/5/src/kudu/tablet/compaction_policy.cc File src/kudu/tablet/compaction_policy.cc: http://gerrit.cloudera.org:8080/#/c/20787/5/src/kudu/tablet/compaction_policy.cc@174 PS5, Line 174: int64_t Could you also please take care of this? Change to uint64_t -- To view, visit http://gerrit.cloudera.org:8080/20787 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5397c7e7bf29942a610b0b8203b6918811c49bbb Gerrit-Change-Number: 20787 Gerrit-PatchSet: 5 Gerrit-Owner: Ádám Bakai Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Ádám Bakai Gerrit-Comment-Date: Tue, 19 Dec 2023 10:38:42 + Gerrit-HasComments: Yes
[kudu-CR] [compaction] Add memory estimation unit test
Ashwani Raina has posted comments on this change. ( http://gerrit.cloudera.org:8080/20787 ) Change subject: [compaction] Add memory estimation unit test .. Patch Set 1: (2 comments) Overall looks good to me. Just one small comment. http://gerrit.cloudera.org:8080/#/c/20787/1/src/kudu/tablet/compaction_policy-test.cc File src/kudu/tablet/compaction_policy-test.cc: http://gerrit.cloudera.org:8080/#/c/20787/1/src/kudu/tablet/compaction_policy-test.cc@647 PS1, Line 647: TestMemoryEstimation You could also add an ASSERT_STR_CONTAINS/ASSERT_STR_NOT_CONTAINS at the end of both conditions to verify that this rowset was/was not picked because of memory budget constraints by matching with this pattern -> "removed from compaction input due to memory constraints" In case you do that, keep in mind that this error log is throttled. So you might see this error just for the first rowset. http://gerrit.cloudera.org:8080/#/c/20787/1/src/kudu/tablet/compaction_policy-test.cc@649 PS1, Line 649: FLAGS_rowset_compaction_memory_estimate_enabled > The idea was that what if it gets enabled by default later? Then the test w Never mind! That should be ok. -- To view, visit http://gerrit.cloudera.org:8080/20787 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I5397c7e7bf29942a610b0b8203b6918811c49bbb Gerrit-Change-Number: 20787 Gerrit-PatchSet: 1 Gerrit-Owner: Ádám Bakai Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Ádám Bakai Gerrit-Comment-Date: Tue, 19 Dec 2023 09:32:24 + Gerrit-HasComments: Yes
[kudu-CR] [compaction] Add memory estimation unit test
Hello Alexey Serbin, Ashwani Raina, Attila Bukor, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20787 to look at the new patch set (#5). Change subject: [compaction] Add memory estimation unit test .. [compaction] Add memory estimation unit test FLAGS_rowset_compaction_memory_estimate_enabled was always set to false during testing, so this part of the code was not executed in testing environment. Since it wasn't executed, it needed some changes so that it can run in testing,too: 1. MockDiskRowSet can not be downcasted to DiskRowSet, so a new function called OnDiskDeltaSize was introduced into RowSet, so now downcasting is not needed anymore and the OnDiskDeltaSize function is used instead. 2. metrics_ is not set up during testing, so it is skipped if it is not initialized. Change-Id: I5397c7e7bf29942a610b0b8203b6918811c49bbb --- M src/kudu/tablet/compaction_policy-test.cc M src/kudu/tablet/compaction_policy.cc M src/kudu/tablet/diskrowset.cc M src/kudu/tablet/diskrowset.h M src/kudu/tablet/memrowset.h M src/kudu/tablet/mock-rowsets.h M src/kudu/tablet/rowset.cc M src/kudu/tablet/rowset.h 8 files changed, 75 insertions(+), 11 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/87/20787/5 -- To view, visit http://gerrit.cloudera.org:8080/20787 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I5397c7e7bf29942a610b0b8203b6918811c49bbb Gerrit-Change-Number: 20787 Gerrit-PatchSet: 5 Gerrit-Owner: Ádám Bakai Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Ádám Bakai
[kudu-CR] [compaction] Add memory estimation unit test
Hello Alexey Serbin, Ashwani Raina, Attila Bukor, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20787 to look at the new patch set (#4). Change subject: [compaction] Add memory estimation unit test .. [compaction] Add memory estimation unit test FLAGS_rowset_compaction_memory_estimate_enabled was always set to false during testing, so this part of the code was not executed in testing environment. Since it wasn't executed, it needed some changes so that it can run in testing,too: 1. MockDiskRowSet can not be downcasted to DiskRowSet, so a new function called OnDiskDeltaSize was introduced into RowSet, so now downcasting is not needed anymore and the OnDiskDeltaSize function is used instead. 2. metrics_ is not set up during testing, so it is skipped if it is not initialized. Change-Id: I5397c7e7bf29942a610b0b8203b6918811c49bbb --- M src/kudu/tablet/compaction_policy-test.cc M src/kudu/tablet/compaction_policy.cc M src/kudu/tablet/diskrowset.cc M src/kudu/tablet/diskrowset.h M src/kudu/tablet/memrowset.h M src/kudu/tablet/mock-rowsets.h M src/kudu/tablet/rowset.cc M src/kudu/tablet/rowset.h 8 files changed, 75 insertions(+), 10 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/87/20787/4 -- To view, visit http://gerrit.cloudera.org:8080/20787 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I5397c7e7bf29942a610b0b8203b6918811c49bbb Gerrit-Change-Number: 20787 Gerrit-PatchSet: 4 Gerrit-Owner: Ádám Bakai Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Ádám Bakai
[kudu-CR] [compaction] Add memory estimation unit test
Hello Alexey Serbin, Ashwani Raina, Attila Bukor, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20787 to look at the new patch set (#3). Change subject: [compaction] Add memory estimation unit test .. [compaction] Add memory estimation unit test FLAGS_rowset_compaction_memory_estimate_enabled was always set to false during testing, so this part of the code was not executed in testing environment. Since it wasn't executed, it needed some changes so that it can run in testing,too: 1. MockDiskRowSet can not be downcasted to DiskRowSet, so a new function called OnDiskDeltaSize was introduced into RowSet, so now downcasting is not needed anymore and the OnDiskDeltaSize function is used instead. 2. metrics_ is not set up during testing, so it is skipped if it is not initialized. Change-Id: I5397c7e7bf29942a610b0b8203b6918811c49bbb --- M src/kudu/tablet/compaction_policy-test.cc M src/kudu/tablet/compaction_policy.cc M src/kudu/tablet/diskrowset.cc M src/kudu/tablet/diskrowset.h M src/kudu/tablet/memrowset.h M src/kudu/tablet/mock-rowsets.h M src/kudu/tablet/rowset.cc M src/kudu/tablet/rowset.h 8 files changed, 75 insertions(+), 10 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/87/20787/3 -- To view, visit http://gerrit.cloudera.org:8080/20787 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I5397c7e7bf29942a610b0b8203b6918811c49bbb Gerrit-Change-Number: 20787 Gerrit-PatchSet: 3 Gerrit-Owner: Ádám Bakai Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Ádám Bakai
[kudu-CR] [compaction] Add memory estimation unit test
Hello Alexey Serbin, Ashwani Raina, Attila Bukor, Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/20787 to look at the new patch set (#2). Change subject: [compaction] Add memory estimation unit test .. [compaction] Add memory estimation unit test FLAGS_rowset_compaction_memory_estimate_enabled was always set to false during testing, so this part of the code was not executed in testing environment. Since it wasn't executed, it needed some changes so that it can run in testing,too: 1. MockDiskRowSet can not be downcasted to DiskRowSet, so a new function called OnDiskDeltaSize was introduced into RowSet, so now downcasting is not needed anymore and the OnDiskDeltaSize function is used instead. 2. metrics_ is not set up during testing, so it is skipped if it is not initialized. Change-Id: I5397c7e7bf29942a610b0b8203b6918811c49bbb --- M src/kudu/tablet/compaction_policy-test.cc M src/kudu/tablet/compaction_policy.cc M src/kudu/tablet/diskrowset.cc M src/kudu/tablet/diskrowset.h M src/kudu/tablet/memrowset.h M src/kudu/tablet/mock-rowsets.h M src/kudu/tablet/rowset.cc M src/kudu/tablet/rowset.h 8 files changed, 74 insertions(+), 9 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/87/20787/2 -- To view, visit http://gerrit.cloudera.org:8080/20787 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I5397c7e7bf29942a610b0b8203b6918811c49bbb Gerrit-Change-Number: 20787 Gerrit-PatchSet: 2 Gerrit-Owner: Ádám Bakai Gerrit-Reviewer: Alexey Serbin Gerrit-Reviewer: Ashwani Raina Gerrit-Reviewer: Attila Bukor Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Ádám Bakai
[kudu-CR] [compaction] Add memory estimation unit test
Ádám Bakai has posted comments on this change. (
http://gerrit.cloudera.org:8080/20787 )
Change subject: [compaction] Add memory estimation unit test
..
Patch Set 1:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/20787/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/20787/1//COMMIT_MSG@7
PS1, Line 7: Add memory estimation unit test
> Maybe add more details about how changing hard limit helps cover test scena
Added comment in the testcase itself.
// Set memory limit to a specified number to make sure that limit is always
hit
// during the test, and it won't depend on actual system memory limit.
http://gerrit.cloudera.org:8080/#/c/20787/1//COMMIT_MSG@9
PS1, Line 9: FLAGS_rowset_compaction_estimate_min_deltas_size_mb
> Did you mean rowset_compaction_memory_estimate_enabled?
Done
http://gerrit.cloudera.org:8080/#/c/20787/1/src/kudu/tablet/compaction_policy-test.cc
File src/kudu/tablet/compaction_policy-test.cc:
http://gerrit.cloudera.org:8080/#/c/20787/1/src/kudu/tablet/compaction_policy-test.cc@649
PS1, Line 649: FLAGS_rowset_compaction_memory_estimate_enabled
> Isn't this disabled by default? No need to set it to false here unless it c
The idea was that what if it gets enabled by default later? Then the test will
fail. Maybe it's better to rewrite the test at that time. If you think it's
better to rewrite I can remove it.
http://gerrit.cloudera.org:8080/#/c/20787/1/src/kudu/tablet/compaction_policy.cc
File src/kudu/tablet/compaction_policy.cc:
http://gerrit.cloudera.org:8080/#/c/20787/1/src/kudu/tablet/compaction_policy.cc@182
PS1, Line 182: {
> Move above to align with other instances.
Done
http://gerrit.cloudera.org:8080/#/c/20787/1/src/kudu/tablet/rowset.cc
File src/kudu/tablet/rowset.cc:
http://gerrit.cloudera.org:8080/#/c/20787/1/src/kudu/tablet/rowset.cc@266
PS1, Line 266: for (const shared_ptr &rs : new_rowsets_)
> OnDiskDeltaSize used in compaction is per rowset. Here it seems to be compu
It's not getting used yet. It was created to follow the pattern in
DuplicatingRowSet, where the the returned value of rowsets from new_rowsets_
are added up and returned.
--
To view, visit http://gerrit.cloudera.org:8080/20787
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5397c7e7bf29942a610b0b8203b6918811c49bbb
Gerrit-Change-Number: 20787
Gerrit-PatchSet: 1
Gerrit-Owner: Ádám Bakai
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Ashwani Raina
Gerrit-Reviewer: Attila Bukor
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Ádám Bakai
Gerrit-Comment-Date: Thu, 14 Dec 2023 11:28:17 +
Gerrit-HasComments: Yes
[kudu-CR] [compaction] Add memory estimation unit test
Ashwani Raina has posted comments on this change. (
http://gerrit.cloudera.org:8080/20787 )
Change subject: [compaction] Add memory estimation unit test
..
Patch Set 1:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/20787/1//COMMIT_MSG
Commit Message:
http://gerrit.cloudera.org:8080/#/c/20787/1//COMMIT_MSG@7
PS1, Line 7: Add memory estimation unit test
Maybe add more details about how changing hard limit helps cover test scenario
for memory budgeting logic.
http://gerrit.cloudera.org:8080/#/c/20787/1//COMMIT_MSG@9
PS1, Line 9: FLAGS_rowset_compaction_estimate_min_deltas_size_mb
Did you mean rowset_compaction_memory_estimate_enabled?
rowset_compaction_estimate_min_deltas_size_mb is for minimum deltas size and
initialised to 64MB default.
http://gerrit.cloudera.org:8080/#/c/20787/1/src/kudu/tablet/compaction_policy-test.cc
File src/kudu/tablet/compaction_policy-test.cc:
http://gerrit.cloudera.org:8080/#/c/20787/1/src/kudu/tablet/compaction_policy-test.cc@649
PS1, Line 649: FLAGS_rowset_compaction_memory_estimate_enabled
Isn't this disabled by default? No need to set it to false here unless it could
be true here for some other reason.
http://gerrit.cloudera.org:8080/#/c/20787/1/src/kudu/tablet/compaction_policy.cc
File src/kudu/tablet/compaction_policy.cc:
http://gerrit.cloudera.org:8080/#/c/20787/1/src/kudu/tablet/compaction_policy.cc@182
PS1, Line 182: {
Move above to align with other instances.
http://gerrit.cloudera.org:8080/#/c/20787/1/src/kudu/tablet/rowset.cc
File src/kudu/tablet/rowset.cc:
http://gerrit.cloudera.org:8080/#/c/20787/1/src/kudu/tablet/rowset.cc@266
PS1, Line 266: for (const shared_ptr &rs : new_rowsets_)
OnDiskDeltaSize used in compaction is per rowset. Here it seems to be computing
total size for all rowsets inside new_rowsets_.
Where is it getting used?
--
To view, visit http://gerrit.cloudera.org:8080/20787
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I5397c7e7bf29942a610b0b8203b6918811c49bbb
Gerrit-Change-Number: 20787
Gerrit-PatchSet: 1
Gerrit-Owner: Ádám Bakai
Gerrit-Reviewer: Alexey Serbin
Gerrit-Reviewer: Ashwani Raina
Gerrit-Reviewer: Attila Bukor
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Comment-Date: Wed, 13 Dec 2023 14:52:50 +
Gerrit-HasComments: Yes
[kudu-CR] [compaction] Add memory estimation unit test
Ádám Bakai has uploaded this change for review. ( http://gerrit.cloudera.org:8080/20787 Change subject: [compaction] Add memory estimation unit test .. [compaction] Add memory estimation unit test FLAGS_rowset_compaction_estimate_min_deltas_size_mb was always set to false during testing, so this part of the code was not executed in testing environment. Since it wasn't executed, it needed some changes so that it can run in testing,too: 1. MockDiskRowSet can not be downcasted to DiskRowSet, so a new function called OnDiskDeltaSize was introduced into RowSet, so now downcasting is not needed anymore and the OnDiskDeltaSize function is used instead. 2. metrics_ is not set up during testing, so it is skipped if it is not initialized. Change-Id: I5397c7e7bf29942a610b0b8203b6918811c49bbb --- M src/kudu/tablet/compaction_policy-test.cc M src/kudu/tablet/compaction_policy.cc M src/kudu/tablet/diskrowset.cc M src/kudu/tablet/diskrowset.h M src/kudu/tablet/memrowset.h M src/kudu/tablet/mock-rowsets.h M src/kudu/tablet/rowset.cc M src/kudu/tablet/rowset.h 8 files changed, 74 insertions(+), 9 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/87/20787/1 -- To view, visit http://gerrit.cloudera.org:8080/20787 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newchange Gerrit-Change-Id: I5397c7e7bf29942a610b0b8203b6918811c49bbb Gerrit-Change-Number: 20787 Gerrit-PatchSet: 1 Gerrit-Owner: Ádám Bakai
