You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Will Berkeley (Code Review)" <ge...@cloudera.org> on 2018/11/02 21:07:23 UTC
[kudu-CR] [compaction] KUDU-1400: Improve rowset compaction policy to consider merging small DRSs
Will Berkeley has uploaded this change for review. ( http://gerrit.cloudera.org:8080/11869
Change subject: [compaction] KUDU-1400: Improve rowset compaction policy to consider merging small DRSs
......................................................................
[compaction] KUDU-1400: Improve rowset compaction policy to consider merging small DRSs
This implements the small rowset compaction scheme explained in the
KUDU-1400 design doc [1]. While the implementation is simple, the
reasoning behind it is nuanced, and I'll defer the explanations of them
to the design doc rather than repeating them here.
The three relevant constant factors:
--compaction_minimum_improvement_score
--compaction_small_rowset_tradeoff
kSupportAdjust
have been tuned to work well with each other. See the relevant tests for
reasoning on why, and validation of what the results should be.
Since compaction policy's performance is important, I ran the YCSB
benchmark before and after the change using 'perf stat' to compare the
performance:
Command:
Before:
Performance counter stats for 'bin/compaction_policy-test --gtest_filter=*Ycsb*' (10 runs):
1231.095442 task-clock # 0.997 CPUs utilized ( +- 2.00% )
148 context-switches # 0.120 K/sec ( +- 2.43% )
9 cpu-migrations # 0.007 K/sec ( +- 16.14% )
3,630 page-faults # 0.003 M/sec ( +- 0.00% )
3,251,530,478 cycles # 2.641 GHz ( +- 2.04% )
<not supported> stalled-cycles-frontend
<not supported> stalled-cycles-backend
5,772,319,429 instructions # 1.78 insns per cycle ( +- 0.01% )
1,070,627,520 branches # 869.654 M/sec ( +- 0.01% )
13,583,368 branch-misses # 1.27% of all branches ( +- 0.10% )
1.235037947 seconds time elapsed ( +- 2.00% )
After:
Performance counter stats for 'bin/compaction_policy-test --gtest_filter=*Ycsb*' (10 runs):
1297.749333 task-clock # 0.994 CPUs utilized ( +- 2.17% )
158 context-switches # 0.122 K/sec ( +- 3.01% )
14 cpu-migrations # 0.011 K/sec ( +- 17.48% )
3,636 page-faults # 0.003 M/sec
3,509,480,140 cycles # 2.704 GHz ( +- 2.65% )
<not supported> stalled-cycles-frontend
<not supported> stalled-cycles-backend
6,800,316,126 instructions # 1.94 insns per cycle ( +- 0.01% )
1,073,111,416 branches # 826.902 M/sec ( +- 0.01% )
13,574,780 branch-misses # 1.26% of all branches ( +- 0.15% )
1.305058206 seconds time elapsed ( +- 2.16% )
A follow up will integrate the below design doc with the existing one,
docs/design-docs/compaction-policy.md.
[1]: https://docs.google.com/document/d/1yTfxt0_2p5EfIjCnjJCt3o-nB9xk-Kl2O8yKTA1LQrQ/edit?usp=sharing
Change-Id: I7b421c6ed77d28ebab9b91a4d6fcb1e825997e6c
---
M src/kudu/tablet/compaction_policy-test.cc
M src/kudu/tablet/compaction_policy.cc
M src/kudu/tablet/rowset_info.cc
M src/kudu/tablet/rowset_info.h
4 files changed, 274 insertions(+), 36 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/69/11869/1
--
To view, visit http://gerrit.cloudera.org:8080/11869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newchange
Gerrit-Change-Id: I7b421c6ed77d28ebab9b91a4d6fcb1e825997e6c
Gerrit-Change-Number: 11869
Gerrit-PatchSet: 1
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
[kudu-CR] [compaction] KUDU-1400: Improve rowset compaction policy to consider merging small DRSs
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11869 )
Change subject: [compaction] KUDU-1400: Improve rowset compaction policy to consider merging small DRSs
......................................................................
[compaction] KUDU-1400: Improve rowset compaction policy to consider merging small DRSs
This implements the small rowset compaction scheme explained in the
KUDU-1400 design doc [1]. While the implementation is simple, the
reasoning behind it is nuanced, and I'll defer the explanations of them
to the design doc rather than repeating them here.
The three relevant constant factors:
--compaction_minimum_improvement_score
--compaction_small_rowset_tradeoff
kSupportAdjust
have been tuned to work well with each other. See the relevant tests for
reasoning on why, and validation of what the results should be.
Since compaction policy's performance is important, I ran the YCSB
benchmark before and after the change using 'perf stat' to compare the
performance:
Command:
Before:
Performance counter stats for 'bin/compaction_policy-test --gtest_filter=*Ycsb*' (10 runs):
1231.095442 task-clock # 0.997 CPUs utilized ( +- 2.00% )
148 context-switches # 0.120 K/sec ( +- 2.43% )
9 cpu-migrations # 0.007 K/sec ( +- 16.14% )
3,630 page-faults # 0.003 M/sec ( +- 0.00% )
3,251,530,478 cycles # 2.641 GHz ( +- 2.04% )
<not supported> stalled-cycles-frontend
<not supported> stalled-cycles-backend
5,772,319,429 instructions # 1.78 insns per cycle ( +- 0.01% )
1,070,627,520 branches # 869.654 M/sec ( +- 0.01% )
13,583,368 branch-misses # 1.27% of all branches ( +- 0.10% )
1.235037947 seconds time elapsed ( +- 2.00% )
After:
Performance counter stats for 'bin/compaction_policy-test --gtest_filter=*Ycsb*' (10 runs):
1297.749333 task-clock # 0.994 CPUs utilized ( +- 2.17% )
158 context-switches # 0.122 K/sec ( +- 3.01% )
14 cpu-migrations # 0.011 K/sec ( +- 17.48% )
3,636 page-faults # 0.003 M/sec
3,509,480,140 cycles # 2.704 GHz ( +- 2.65% )
<not supported> stalled-cycles-frontend
<not supported> stalled-cycles-backend
6,800,316,126 instructions # 1.94 insns per cycle ( +- 0.01% )
1,073,111,416 branches # 826.902 M/sec ( +- 0.01% )
13,574,780 branch-misses # 1.26% of all branches ( +- 0.15% )
1.305058206 seconds time elapsed ( +- 2.16% )
A follow up will integrate the below design doc with the existing one,
docs/design-docs/compaction-policy.md.
[1]: https://docs.google.com/document/d/1yTfxt0_2p5EfIjCnjJCt3o-nB9xk-Kl2O8yKTA1LQrQ/edit?usp=sharing
Change-Id: I7b421c6ed77d28ebab9b91a4d6fcb1e825997e6c
Reviewed-on: http://gerrit.cloudera.org:8080/11869
Tested-by: Kudu Jenkins
Reviewed-by: Andrew Wong <aw...@cloudera.com>
Reviewed-by: Alexey Serbin <as...@cloudera.com>
---
M src/kudu/tablet/compaction_policy-test.cc
M src/kudu/tablet/compaction_policy.cc
M src/kudu/tablet/rowset_info.cc
M src/kudu/tablet/rowset_info.h
4 files changed, 297 insertions(+), 45 deletions(-)
Approvals:
Kudu Jenkins: Verified
Andrew Wong: Looks good to me, approved
Alexey Serbin: Looks good to me, approved
--
To view, visit http://gerrit.cloudera.org:8080/11869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: merged
Gerrit-Change-Id: I7b421c6ed77d28ebab9b91a4d6fcb1e825997e6c
Gerrit-Change-Number: 11869
Gerrit-PatchSet: 6
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] [compaction] KUDU-1400: Improve rowset compaction policy to consider merging small DRSs
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11869 )
Change subject: [compaction] KUDU-1400: Improve rowset compaction policy to consider merging small DRSs
......................................................................
Patch Set 3:
The alter table randomized failure looks unrelated but like it could be a real thing: a tablet failed because it's cmeta was missing after its tablet server was restarted. The master was trying to create the tablet replica on the server at the time- the tablet server was trying to create the tablet when it crashed and it ended up with missing cmeta. The cluster tried to copy over the failed tablet replica but couldn't. I think there might be an issue filed for this already.
--
To view, visit http://gerrit.cloudera.org:8080/11869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7b421c6ed77d28ebab9b91a4d6fcb1e825997e6c
Gerrit-Change-Number: 11869
Gerrit-PatchSet: 3
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Sat, 03 Nov 2018 00:07:05 +0000
Gerrit-HasComments: No
[kudu-CR] [compaction] KUDU-1400: Improve rowset compaction policy to consider merging small DRSs
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11869 )
Change subject: [compaction] KUDU-1400: Improve rowset compaction policy to consider merging small DRSs
......................................................................
Patch Set 5: Code-Review+2
(2 comments)
http://gerrit.cloudera.org:8080/#/c/11869/4/src/kudu/tablet/compaction_policy-test.cc
File src/kudu/tablet/compaction_policy-test.cc:
http://gerrit.cloudera.org:8080/#/c/11869/4/src/kudu/tablet/compaction_policy-test.cc@321
PS4, Line 321: if doing so
: // doesn't actually reduce the number of rowsets
> An original test, TestBudgetedSelection, tests the case of a small number o
SGTM
http://gerrit.cloudera.org:8080/#/c/11869/4/src/kudu/tablet/compaction_policy-test.cc@381
PS4, Line 381: if (divisor > 2) {
: ASSERT_EQ(rowsets.size(), picked.size());
> Yeah, what I was trying to say in my previous comment was that the existing
SGTM
--
To view, visit http://gerrit.cloudera.org:8080/11869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7b421c6ed77d28ebab9b91a4d6fcb1e825997e6c
Gerrit-Change-Number: 11869
Gerrit-PatchSet: 5
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 20 Nov 2018 16:58:41 +0000
Gerrit-HasComments: Yes
[kudu-CR] [compaction] KUDU-1400: Improve rowset compaction policy to consider merging small DRSs
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11869 )
Change subject: [compaction] KUDU-1400: Improve rowset compaction policy to consider merging small DRSs
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11869/4/src/kudu/tablet/compaction_policy-test.cc
File src/kudu/tablet/compaction_policy-test.cc:
http://gerrit.cloudera.org:8080/#/c/11869/4/src/kudu/tablet/compaction_policy-test.cc@381
PS4, Line 381: if (divisor > 2) {
: ASSERT_EQ(rowsets.size(), picked.size());
> Yep, I understand the general case. That was more about having explicit sc
Yeah, what I was trying to say in my previous comment was that the existing test cases exhaust most of the "wiggle room" on the constants. Adding a new test case doesn't specify the behavior more.
--
To view, visit http://gerrit.cloudera.org:8080/11869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7b421c6ed77d28ebab9b91a4d6fcb1e825997e6c
Gerrit-Change-Number: 11869
Gerrit-PatchSet: 4
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Tue, 20 Nov 2018 07:02:36 +0000
Gerrit-HasComments: Yes
[kudu-CR] [compaction] KUDU-1400: Improve rowset compaction policy to consider merging small DRSs
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11869 )
Change subject: [compaction] KUDU-1400: Improve rowset compaction policy to consider merging small DRSs
......................................................................
Patch Set 4:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11869/4/src/kudu/tablet/compaction_policy-test.cc
File src/kudu/tablet/compaction_policy-test.cc:
http://gerrit.cloudera.org:8080/#/c/11869/4/src/kudu/tablet/compaction_policy-test.cc@381
PS4, Line 381: if (divisor > 2) {
: ASSERT_EQ(rowsets.size(), picked.size());
> You're asking if 3 adjacent rowsets of size 32/3 MB each should be compacte
Yep, I was asking about 3 adjacent rowsets of size 32/3 MB each. The idea was: if we expect those to be compacted, why not to add that divisor (i.e. 3) into the list and have that case covered by this scenario as well.
--
To view, visit http://gerrit.cloudera.org:8080/11869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7b421c6ed77d28ebab9b91a4d6fcb1e825997e6c
Gerrit-Change-Number: 11869
Gerrit-PatchSet: 4
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Fri, 16 Nov 2018 21:11:27 +0000
Gerrit-HasComments: Yes
[kudu-CR] [compaction] KUDU-1400: Improve rowset compaction policy to consider merging small DRSs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11869 )
Change subject: [compaction] KUDU-1400: Improve rowset compaction policy to consider merging small DRSs
......................................................................
Patch Set 5: Code-Review+2
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11869/4/src/kudu/tablet/compaction_policy-test.cc
File src/kudu/tablet/compaction_policy-test.cc:
http://gerrit.cloudera.org:8080/#/c/11869/4/src/kudu/tablet/compaction_policy-test.cc@381
PS4, Line 381: if (divisor > 2) {
: ASSERT_EQ(rowsets.size(), picked.size());
> Yep, I was asking about 3 adjacent rowsets of size 32/3 MB each. The idea
Ah I thought you were pondering the general case, rather than 3 specifically.
--
To view, visit http://gerrit.cloudera.org:8080/11869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7b421c6ed77d28ebab9b91a4d6fcb1e825997e6c
Gerrit-Change-Number: 11869
Gerrit-PatchSet: 5
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Fri, 16 Nov 2018 21:29:03 +0000
Gerrit-HasComments: Yes
[kudu-CR] [compaction] KUDU-1400: Improve rowset compaction policy to consider merging small DRSs
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11869 )
Change subject: [compaction] KUDU-1400: Improve rowset compaction policy to consider merging small DRSs
......................................................................
Patch Set 4:
(5 comments)
Looks good to me. A few nits and one question.
http://gerrit.cloudera.org:8080/#/c/11869/4/src/kudu/tablet/compaction_policy-test.cc
File src/kudu/tablet/compaction_policy-test.cc:
http://gerrit.cloudera.org:8080/#/c/11869/4/src/kudu/tablet/compaction_policy-test.cc@321
PS4, Line 321: if doing so
: // doesn't actually reduce the number of rowsets
Does it make sense to add a test to see how the compaction algorithm behaves when there are many overlapping smallish rowsets when target size is order of magnitude higher? I might be missing something, but I don't see such a test among the existing ones.
http://gerrit.cloudera.org:8080/#/c/11869/4/src/kudu/tablet/compaction_policy-test.cc@336
PS4, Line 336: [0 - 1]
nit: it seems this one is extra -- the index starts with 1. Instead, it seem there should be [99-100] trailing one. Maybe, update the lower index range instead.
http://gerrit.cloudera.org:8080/#/c/11869/4/src/kudu/tablet/compaction_policy-test.cc@345
PS4, Line 345: ASSERT_EQ(quality, 0.0);
nit: the expected values comes first for XXX_EQ() test macros
http://gerrit.cloudera.org:8080/#/c/11869/4/src/kudu/tablet/compaction_policy-test.cc@381
PS4, Line 381: if (divisor > 2) {
: ASSERT_EQ(rowsets.size(), picked.size());
Should it also hold if the divisor is 3 in case of 3 rowsets? Or it doesn't matter?
http://gerrit.cloudera.org:8080/#/c/11869/4/src/kudu/tablet/compaction_policy-test.cc@391
PS4, Line 391: FLAGS_budgeted_compaction_target_rowset_size
nit: does it make sense to use 'target_size_bytes' as well?
--
To view, visit http://gerrit.cloudera.org:8080/11869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7b421c6ed77d28ebab9b91a4d6fcb1e825997e6c
Gerrit-Change-Number: 11869
Gerrit-PatchSet: 4
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 14 Nov 2018 19:42:41 +0000
Gerrit-HasComments: Yes
[kudu-CR] [compaction] KUDU-1400: Improve rowset compaction policy to consider merging small DRSs
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11869 )
Change subject: [compaction] KUDU-1400: Improve rowset compaction policy to consider merging small DRSs
......................................................................
Patch Set 4:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/11869/4/src/kudu/tablet/compaction_policy-test.cc
File src/kudu/tablet/compaction_policy-test.cc:
http://gerrit.cloudera.org:8080/#/c/11869/4/src/kudu/tablet/compaction_policy-test.cc@381
PS4, Line 381: if (divisor > 2) {
: ASSERT_EQ(rowsets.size(), picked.size());
> Ah I thought you were pondering the general case, rather than 3 specificall
Yep, I understand the general case. That was more about having explicit scenarios for the cases which are closer to the 'switch-the-compaction-behavior' boundaries. If those alpha and gamma constants are to be updated at some point, I think it's better have a snapshot on the expected behavior to spot as many regressions as possible.
http://gerrit.cloudera.org:8080/#/c/11869/5/src/kudu/tablet/compaction_policy-test.cc
File src/kudu/tablet/compaction_policy-test.cc:
http://gerrit.cloudera.org:8080/#/c/11869/5/src/kudu/tablet/compaction_policy-test.cc@362
PS5, Line 362: { 32, 16, 8, 4, 2, 1 }
So, no need to have the coverage for the divisor of 3?
--
To view, visit http://gerrit.cloudera.org:8080/11869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7b421c6ed77d28ebab9b91a4d6fcb1e825997e6c
Gerrit-Change-Number: 11869
Gerrit-PatchSet: 4
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Sat, 17 Nov 2018 02:47:19 +0000
Gerrit-HasComments: Yes
[kudu-CR] [compaction] KUDU-1400: Improve rowset compaction policy to consider merging small DRSs
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11869 )
Change subject: [compaction] KUDU-1400: Improve rowset compaction policy to consider merging small DRSs
......................................................................
Patch Set 4:
(14 comments)
http://gerrit.cloudera.org:8080/#/c/11869/4/src/kudu/tablet/compaction_policy-test.cc
File src/kudu/tablet/compaction_policy-test.cc:
http://gerrit.cloudera.org:8080/#/c/11869/4/src/kudu/tablet/compaction_policy-test.cc@321
PS4, Line 321: if doing so
: // doesn't actually reduce the number of rowsets
> Does it make sense to add a test to see how the compaction algorithm behave
An original test, TestBudgetedSelection, tests the case of a small number of overlapping rowsets. I don't think having many rowsets overlapping tests the code in any new way beyond having 3.
http://gerrit.cloudera.org:8080/#/c/11869/4/src/kudu/tablet/compaction_policy-test.cc@336
PS4, Line 336: [0 - 1]
> nit: it seems this one is extra -- the index starts with 1. Instead, it se
Done
http://gerrit.cloudera.org:8080/#/c/11869/4/src/kudu/tablet/compaction_policy-test.cc@345
PS4, Line 345: ASSERT_EQ(quality, 0.0);
> nit: the expected values comes first for XXX_EQ() test macros
Done. Here and elsewhere.
http://gerrit.cloudera.org:8080/#/c/11869/4/src/kudu/tablet/compaction_policy-test.cc@344
PS4, Line 344: ASSERT_TRUE(picked.empty());
: ASSERT_EQ(quality, 0.0);
> nit: think it's worth using EXPECT* here so we can run over all rowsets, ev
No, because the expectation for the loops is that if it fails one one iteration, it fails for all subsequent ones, so the extra EXPECT_* failures would just be noise.
http://gerrit.cloudera.org:8080/#/c/11869/4/src/kudu/tablet/compaction_policy-test.cc@381
PS4, Line 381: if (divisor > 2) {
: ASSERT_EQ(rowsets.size(), picked.size());
> I think that isn't what we want. As the number of rowsets increases, the (N
I think Alexey is asking about something else.
http://gerrit.cloudera.org:8080/#/c/11869/4/src/kudu/tablet/compaction_policy-test.cc@381
PS4, Line 381: if (divisor > 2) {
: ASSERT_EQ(rowsets.size(), picked.size());
> Should it also hold if the divisor is 3 in case of 3 rowsets? Or it doesn'
You're asking if 3 adjacent rowsets of size 32/3 MB each should be compacted together. That's a decision about how important we think small rowset compaction is. Maybe they should be, maybe not, but the various choices I'm enforcing in this test have already exhausted most of my ability to tune the weights and coefficients, and think they represent reasonable choices, so whatever actually does happen is fine with me.
http://gerrit.cloudera.org:8080/#/c/11869/4/src/kudu/tablet/compaction_policy-test.cc@391
PS4, Line 391: FLAGS_budgeted_compaction_target_rowset_size
> nit: does it make sense to use 'target_size_bytes' as well?
Done
http://gerrit.cloudera.org:8080/#/c/11869/4/src/kudu/tablet/compaction_policy.cc
File src/kudu/tablet/compaction_policy.cc:
http://gerrit.cloudera.org:8080/#/c/11869/4/src/kudu/tablet/compaction_policy.cc@126
PS4, Line 126: const RowSetInfo*
> nit: not related to this particular patch, but if item_type is typedef'ed,
Done
http://gerrit.cloudera.org:8080/#/c/11869/4/src/kudu/tablet/compaction_policy.cc@268
PS4, Line 268: double MaybePenalizeWideSolution(double sum_width, double union_width) {
: return sum_width <= union_width ? sum_width - union_width :
: sum_width - kSupportAdjust * union_width;
: }
> This is worth a comment. My attempt:
Done
http://gerrit.cloudera.org:8080/#/c/11869/4/src/kudu/tablet/rowset_info.h
File src/kudu/tablet/rowset_info.h:
http://gerrit.cloudera.org:8080/#/c/11869/4/src/kudu/tablet/rowset_info.h@87
PS4, Line 87: by this RowSet
> nit: not your fault, but mind tweaking this to be "is spanned by the min/ma
Done
http://gerrit.cloudera.org:8080/#/c/11869/4/src/kudu/tablet/rowset_info.cc
File src/kudu/tablet/rowset_info.cc:
http://gerrit.cloudera.org:8080/#/c/11869/4/src/kudu/tablet/rowset_info.cc@65
PS4, Line 65: compaction_small_rowset_tradeoff
> +1, same above?
Done
http://gerrit.cloudera.org:8080/#/c/11869/4/src/kudu/tablet/rowset_info.cc@65
PS4, Line 65: compaction_small_rowset_tradeoff
> nit: is it worth marking this flag with the 'runtime' tag?
Done
http://gerrit.cloudera.org:8080/#/c/11869/4/src/kudu/tablet/rowset_info.cc@90
PS4, Line 90: --compaction_small_rowset_tradeoff=$0 must be less than "
: "--compaction_minimum_improvement=$1 in order to prevent pointless "
: "compactions; if you know what you are doing, pass "
: "--compaction_force_small_rowset_tradeoff to permit this",
> nit: consistent -flag_dashes with the above flag?
Done
http://gerrit.cloudera.org:8080/#/c/11869/4/src/kudu/tablet/rowset_info.cc@230
PS4, Line 230: const auto size_score =
: 1 - static_cast<double>(size_bytes) / target_size_bytes;
> I understand the sentiment behind wanting to just point to a design doc for
Done
--
To view, visit http://gerrit.cloudera.org:8080/11869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7b421c6ed77d28ebab9b91a4d6fcb1e825997e6c
Gerrit-Change-Number: 11869
Gerrit-PatchSet: 4
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Fri, 16 Nov 2018 19:06:08 +0000
Gerrit-HasComments: Yes
[kudu-CR] [compaction] KUDU-1400: Improve rowset compaction policy to consider merging small DRSs
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11869 )
Change subject: [compaction] KUDU-1400: Improve rowset compaction policy to consider merging small DRSs
......................................................................
Patch Set 3:
I'm not sure about the kudu-tool-test failure. TSAN reports a deadlock on a rowset's compact_flush_lock, but there's not much more information than that. This patch touches code close to that, but it doesn't actually change anything that AFAICT interacts with locking, so not sure what is going on here.
--
To view, visit http://gerrit.cloudera.org:8080/11869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7b421c6ed77d28ebab9b91a4d6fcb1e825997e6c
Gerrit-Change-Number: 11869
Gerrit-PatchSet: 3
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Fri, 02 Nov 2018 23:59:33 +0000
Gerrit-HasComments: No
[kudu-CR] [compaction] KUDU-1400: Improve rowset compaction policy to consider merging small DRSs
Posted by "Andrew Wong (Code Review)" <ge...@cloudera.org>.
Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/11869 )
Change subject: [compaction] KUDU-1400: Improve rowset compaction policy to consider merging small DRSs
......................................................................
Patch Set 4:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/11869/4/src/kudu/tablet/compaction_policy-test.cc
File src/kudu/tablet/compaction_policy-test.cc:
http://gerrit.cloudera.org:8080/#/c/11869/4/src/kudu/tablet/compaction_policy-test.cc@344
PS4, Line 344: ASSERT_TRUE(picked.empty());
: ASSERT_EQ(quality, 0.0);
nit: think it's worth using EXPECT* here so we can run over all rowsets, even upon failure? Same for other tests with loops.
http://gerrit.cloudera.org:8080/#/c/11869/4/src/kudu/tablet/compaction_policy-test.cc@381
PS4, Line 381: if (divisor > 2) {
: ASSERT_EQ(rowsets.size(), picked.size());
> Should it also hold if the divisor is 3 in case of 3 rowsets? Or it doesn'
I think that isn't what we want. As the number of rowsets increases, the (Ni-Nf) would increase the score. Take the example of 100 non-overlapping rowsets: we would definitely want to compact, right?
http://gerrit.cloudera.org:8080/#/c/11869/4/src/kudu/tablet/compaction_policy.cc
File src/kudu/tablet/compaction_policy.cc:
http://gerrit.cloudera.org:8080/#/c/11869/4/src/kudu/tablet/compaction_policy.cc@268
PS4, Line 268: double MaybePenalizeWideSolution(double sum_width, double union_width) {
: return sum_width <= union_width ? sum_width - union_width :
: sum_width - kSupportAdjust * union_width;
: }
This is worth a comment. My attempt:
In cases where we are reducing the overlap, we try to favor narrower solutions, lest we attempt to include an excessive number of rowsets for the same overlap-reduction benefit. To do so, we tweak the calculation to give narrow solutions a larger score.
http://gerrit.cloudera.org:8080/#/c/11869/4/src/kudu/tablet/rowset_info.h
File src/kudu/tablet/rowset_info.h:
http://gerrit.cloudera.org:8080/#/c/11869/4/src/kudu/tablet/rowset_info.h@87
PS4, Line 87: by this RowSet
nit: not your fault, but mind tweaking this to be "is spanned by the min/max bounds of this RowSet"? Right now, this comment seems a little nuanced, and worth clarifying.
http://gerrit.cloudera.org:8080/#/c/11869/4/src/kudu/tablet/rowset_info.cc
File src/kudu/tablet/rowset_info.cc:
http://gerrit.cloudera.org:8080/#/c/11869/4/src/kudu/tablet/rowset_info.cc@65
PS4, Line 65: compaction_small_rowset_tradeoff
> nit: is it worth marking this flag with the 'runtime' tag?
+1, same above?
http://gerrit.cloudera.org:8080/#/c/11869/4/src/kudu/tablet/rowset_info.cc@90
PS4, Line 90: --compaction_small_rowset_tradeoff=$0 must be less than "
: "--compaction_minimum_improvement=$1 in order to prevent pointless "
: "compactions; if you know what you are doing, pass "
: "--compaction_force_small_rowset_tradeoff to permit this",
nit: consistent -flag_dashes with the above flag?
http://gerrit.cloudera.org:8080/#/c/11869/4/src/kudu/tablet/rowset_info.cc@230
PS4, Line 230: const auto size_score =
: 1 - static_cast<double>(size_bytes) / target_size_bytes;
I understand the sentiment behind wanting to just point to a design doc for this, but I think it'd be really helpful to at least give a one-two line comment about this. This is an approximation for the expected reduction in rowset count that the given rowset will provide, and I don't think that idea is captured by calling it a "size_score".
Also, in the doc, `alpha` is `gamma`. Mind keeping that consistent here? It's kind of confusing otherwise.
--
To view, visit http://gerrit.cloudera.org:8080/11869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7b421c6ed77d28ebab9b91a4d6fcb1e825997e6c
Gerrit-Change-Number: 11869
Gerrit-PatchSet: 4
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Wed, 14 Nov 2018 20:13:44 +0000
Gerrit-HasComments: Yes
[kudu-CR] [compaction] KUDU-1400: Improve rowset compaction policy to consider merging small DRSs
Posted by "Alexey Serbin (Code Review)" <ge...@cloudera.org>.
Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/11869 )
Change subject: [compaction] KUDU-1400: Improve rowset compaction policy to consider merging small DRSs
......................................................................
Patch Set 4:
(2 comments)
Looked a bit into the code, so far just a few nits. I don't understand the whole picture yet, but I'll try to digest the design doc and look once more over the weekend.
http://gerrit.cloudera.org:8080/#/c/11869/4/src/kudu/tablet/compaction_policy.cc
File src/kudu/tablet/compaction_policy.cc:
http://gerrit.cloudera.org:8080/#/c/11869/4/src/kudu/tablet/compaction_policy.cc@126
PS4, Line 126: const RowSetInfo*
nit: not related to this particular patch, but if item_type is typedef'ed, why not to use it in the signature of get_weight() and get_value() functions?
http://gerrit.cloudera.org:8080/#/c/11869/4/src/kudu/tablet/rowset_info.cc
File src/kudu/tablet/rowset_info.cc:
http://gerrit.cloudera.org:8080/#/c/11869/4/src/kudu/tablet/rowset_info.cc@65
PS4, Line 65: compaction_small_rowset_tradeoff
nit: is it worth marking this flag with the 'runtime' tag?
--
To view, visit http://gerrit.cloudera.org:8080/11869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7b421c6ed77d28ebab9b91a4d6fcb1e825997e6c
Gerrit-Change-Number: 11869
Gerrit-PatchSet: 4
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Fri, 09 Nov 2018 01:39:17 +0000
Gerrit-HasComments: Yes
[kudu-CR] [compaction] KUDU-1400: Improve rowset compaction policy to consider merging small DRSs
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11869 )
Change subject: [compaction] KUDU-1400: Improve rowset compaction policy to consider merging small DRSs
......................................................................
Patch Set 3:
> I need to check how the undos might have done away.
We do this as part of merge compaction. So either we should probably turn off rowset compaction for the test, since we specifically want to test the undo delta block GC MM op.
--
To view, visit http://gerrit.cloudera.org:8080/11869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7b421c6ed77d28ebab9b91a4d6fcb1e825997e6c
Gerrit-Change-Number: 11869
Gerrit-PatchSet: 3
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Fri, 02 Nov 2018 23:28:31 +0000
Gerrit-HasComments: No
[kudu-CR] [compaction] KUDU-1400: Improve rowset compaction policy to consider merging small DRSs
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11869 )
Change subject: [compaction] KUDU-1400: Improve rowset compaction policy to consider merging small DRSs
......................................................................
Patch Set 3:
(1 comment)
http://gerrit.cloudera.org:8080/#/c/11869/1/src/kudu/tablet/rowset_info.cc
File src/kudu/tablet/rowset_info.cc:
http://gerrit.cloudera.org:8080/#/c/11869/1/src/kudu/tablet/rowset_info.cc@32
PS1, Line 32:
> warning: #includes are not sorted properly [llvm-include-order]
Done
--
To view, visit http://gerrit.cloudera.org:8080/11869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7b421c6ed77d28ebab9b91a4d6fcb1e825997e6c
Gerrit-Change-Number: 11869
Gerrit-PatchSet: 3
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Fri, 02 Nov 2018 21:28:52 +0000
Gerrit-HasComments: Yes
[kudu-CR] [compaction] KUDU-1400: Improve rowset compaction policy to consider merging small DRSs
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11869 )
Change subject: [compaction] KUDU-1400: Improve rowset compaction policy to consider merging small DRSs
......................................................................
Patch Set 3:
> I'm not sure about the kudu-tool-test failure. TSAN reports a
> deadlock on a rowset's compact_flush_lock
It's not a deadlock. Instead, the test flushes many rowsets and, as expected, the new rowset compaction wants to compact them. There end up being so many to compact at once (60+) that taking all the rowsets compact_flush_locks overflows the fixed (64) size of an array that the TSAN code uses as part of tracking lock acquisitions and their order.
--
To view, visit http://gerrit.cloudera.org:8080/11869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7b421c6ed77d28ebab9b91a4d6fcb1e825997e6c
Gerrit-Change-Number: 11869
Gerrit-PatchSet: 3
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Mon, 05 Nov 2018 23:02:33 +0000
Gerrit-HasComments: No
[kudu-CR] [compaction] KUDU-1400: Improve rowset compaction policy to consider merging small DRSs
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11869
to look at the new patch set (#5).
Change subject: [compaction] KUDU-1400: Improve rowset compaction policy to consider merging small DRSs
......................................................................
[compaction] KUDU-1400: Improve rowset compaction policy to consider merging small DRSs
This implements the small rowset compaction scheme explained in the
KUDU-1400 design doc [1]. While the implementation is simple, the
reasoning behind it is nuanced, and I'll defer the explanations of them
to the design doc rather than repeating them here.
The three relevant constant factors:
--compaction_minimum_improvement_score
--compaction_small_rowset_tradeoff
kSupportAdjust
have been tuned to work well with each other. See the relevant tests for
reasoning on why, and validation of what the results should be.
Since compaction policy's performance is important, I ran the YCSB
benchmark before and after the change using 'perf stat' to compare the
performance:
Command:
Before:
Performance counter stats for 'bin/compaction_policy-test --gtest_filter=*Ycsb*' (10 runs):
1231.095442 task-clock # 0.997 CPUs utilized ( +- 2.00% )
148 context-switches # 0.120 K/sec ( +- 2.43% )
9 cpu-migrations # 0.007 K/sec ( +- 16.14% )
3,630 page-faults # 0.003 M/sec ( +- 0.00% )
3,251,530,478 cycles # 2.641 GHz ( +- 2.04% )
<not supported> stalled-cycles-frontend
<not supported> stalled-cycles-backend
5,772,319,429 instructions # 1.78 insns per cycle ( +- 0.01% )
1,070,627,520 branches # 869.654 M/sec ( +- 0.01% )
13,583,368 branch-misses # 1.27% of all branches ( +- 0.10% )
1.235037947 seconds time elapsed ( +- 2.00% )
After:
Performance counter stats for 'bin/compaction_policy-test --gtest_filter=*Ycsb*' (10 runs):
1297.749333 task-clock # 0.994 CPUs utilized ( +- 2.17% )
158 context-switches # 0.122 K/sec ( +- 3.01% )
14 cpu-migrations # 0.011 K/sec ( +- 17.48% )
3,636 page-faults # 0.003 M/sec
3,509,480,140 cycles # 2.704 GHz ( +- 2.65% )
<not supported> stalled-cycles-frontend
<not supported> stalled-cycles-backend
6,800,316,126 instructions # 1.94 insns per cycle ( +- 0.01% )
1,073,111,416 branches # 826.902 M/sec ( +- 0.01% )
13,574,780 branch-misses # 1.26% of all branches ( +- 0.15% )
1.305058206 seconds time elapsed ( +- 2.16% )
A follow up will integrate the below design doc with the existing one,
docs/design-docs/compaction-policy.md.
[1]: https://docs.google.com/document/d/1yTfxt0_2p5EfIjCnjJCt3o-nB9xk-Kl2O8yKTA1LQrQ/edit?usp=sharing
Change-Id: I7b421c6ed77d28ebab9b91a4d6fcb1e825997e6c
---
M src/kudu/tablet/compaction_policy-test.cc
M src/kudu/tablet/compaction_policy.cc
M src/kudu/tablet/rowset_info.cc
M src/kudu/tablet/rowset_info.h
4 files changed, 297 insertions(+), 45 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/69/11869/5
--
To view, visit http://gerrit.cloudera.org:8080/11869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7b421c6ed77d28ebab9b91a4d6fcb1e825997e6c
Gerrit-Change-Number: 11869
Gerrit-PatchSet: 5
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
[kudu-CR] [compaction] KUDU-1400: Improve rowset compaction policy to consider merging small DRSs
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11869
to look at the new patch set (#3).
Change subject: [compaction] KUDU-1400: Improve rowset compaction policy to consider merging small DRSs
......................................................................
[compaction] KUDU-1400: Improve rowset compaction policy to consider merging small DRSs
This implements the small rowset compaction scheme explained in the
KUDU-1400 design doc [1]. While the implementation is simple, the
reasoning behind it is nuanced, and I'll defer the explanations of them
to the design doc rather than repeating them here.
The three relevant constant factors:
--compaction_minimum_improvement_score
--compaction_small_rowset_tradeoff
kSupportAdjust
have been tuned to work well with each other. See the relevant tests for
reasoning on why, and validation of what the results should be.
Since compaction policy's performance is important, I ran the YCSB
benchmark before and after the change using 'perf stat' to compare the
performance:
Command:
Before:
Performance counter stats for 'bin/compaction_policy-test --gtest_filter=*Ycsb*' (10 runs):
1231.095442 task-clock # 0.997 CPUs utilized ( +- 2.00% )
148 context-switches # 0.120 K/sec ( +- 2.43% )
9 cpu-migrations # 0.007 K/sec ( +- 16.14% )
3,630 page-faults # 0.003 M/sec ( +- 0.00% )
3,251,530,478 cycles # 2.641 GHz ( +- 2.04% )
<not supported> stalled-cycles-frontend
<not supported> stalled-cycles-backend
5,772,319,429 instructions # 1.78 insns per cycle ( +- 0.01% )
1,070,627,520 branches # 869.654 M/sec ( +- 0.01% )
13,583,368 branch-misses # 1.27% of all branches ( +- 0.10% )
1.235037947 seconds time elapsed ( +- 2.00% )
After:
Performance counter stats for 'bin/compaction_policy-test --gtest_filter=*Ycsb*' (10 runs):
1297.749333 task-clock # 0.994 CPUs utilized ( +- 2.17% )
158 context-switches # 0.122 K/sec ( +- 3.01% )
14 cpu-migrations # 0.011 K/sec ( +- 17.48% )
3,636 page-faults # 0.003 M/sec
3,509,480,140 cycles # 2.704 GHz ( +- 2.65% )
<not supported> stalled-cycles-frontend
<not supported> stalled-cycles-backend
6,800,316,126 instructions # 1.94 insns per cycle ( +- 0.01% )
1,073,111,416 branches # 826.902 M/sec ( +- 0.01% )
13,574,780 branch-misses # 1.26% of all branches ( +- 0.15% )
1.305058206 seconds time elapsed ( +- 2.16% )
A follow up will integrate the below design doc with the existing one,
docs/design-docs/compaction-policy.md.
[1]: https://docs.google.com/document/d/1yTfxt0_2p5EfIjCnjJCt3o-nB9xk-Kl2O8yKTA1LQrQ/edit?usp=sharing
Change-Id: I7b421c6ed77d28ebab9b91a4d6fcb1e825997e6c
---
M src/kudu/tablet/compaction_policy-test.cc
M src/kudu/tablet/compaction_policy.cc
M src/kudu/tablet/rowset_info.cc
M src/kudu/tablet/rowset_info.h
4 files changed, 278 insertions(+), 37 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/69/11869/3
--
To view, visit http://gerrit.cloudera.org:8080/11869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7b421c6ed77d28ebab9b91a4d6fcb1e825997e6c
Gerrit-Change-Number: 11869
Gerrit-PatchSet: 3
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [compaction] KUDU-1400: Improve rowset compaction policy to consider merging small DRSs
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Hello Tidy Bot, Alexey Serbin, Kudu Jenkins, Andrew Wong, Adar Dembo, Todd Lipcon,
I'd like you to reexamine a change. Please visit
http://gerrit.cloudera.org:8080/11869
to look at the new patch set (#2).
Change subject: [compaction] KUDU-1400: Improve rowset compaction policy to consider merging small DRSs
......................................................................
[compaction] KUDU-1400: Improve rowset compaction policy to consider merging small DRSs
This implements the small rowset compaction scheme explained in the
KUDU-1400 design doc [1]. While the implementation is simple, the
reasoning behind it is nuanced, and I'll defer the explanations of them
to the design doc rather than repeating them here.
The three relevant constant factors:
--compaction_minimum_improvement_score
--compaction_small_rowset_tradeoff
kSupportAdjust
have been tuned to work well with each other. See the relevant tests for
reasoning on why, and validation of what the results should be.
Since compaction policy's performance is important, I ran the YCSB
benchmark before and after the change using 'perf stat' to compare the
performance:
Command:
Before:
Performance counter stats for 'bin/compaction_policy-test --gtest_filter=*Ycsb*' (10 runs):
1231.095442 task-clock # 0.997 CPUs utilized ( +- 2.00% )
148 context-switches # 0.120 K/sec ( +- 2.43% )
9 cpu-migrations # 0.007 K/sec ( +- 16.14% )
3,630 page-faults # 0.003 M/sec ( +- 0.00% )
3,251,530,478 cycles # 2.641 GHz ( +- 2.04% )
<not supported> stalled-cycles-frontend
<not supported> stalled-cycles-backend
5,772,319,429 instructions # 1.78 insns per cycle ( +- 0.01% )
1,070,627,520 branches # 869.654 M/sec ( +- 0.01% )
13,583,368 branch-misses # 1.27% of all branches ( +- 0.10% )
1.235037947 seconds time elapsed ( +- 2.00% )
After:
Performance counter stats for 'bin/compaction_policy-test --gtest_filter=*Ycsb*' (10 runs):
1297.749333 task-clock # 0.994 CPUs utilized ( +- 2.17% )
158 context-switches # 0.122 K/sec ( +- 3.01% )
14 cpu-migrations # 0.011 K/sec ( +- 17.48% )
3,636 page-faults # 0.003 M/sec
3,509,480,140 cycles # 2.704 GHz ( +- 2.65% )
<not supported> stalled-cycles-frontend
<not supported> stalled-cycles-backend
6,800,316,126 instructions # 1.94 insns per cycle ( +- 0.01% )
1,073,111,416 branches # 826.902 M/sec ( +- 0.01% )
13,574,780 branch-misses # 1.26% of all branches ( +- 0.15% )
1.305058206 seconds time elapsed ( +- 2.16% )
A follow up will integrate the below design doc with the existing one,
docs/design-docs/compaction-policy.md.
[1]: https://docs.google.com/document/d/1yTfxt0_2p5EfIjCnjJCt3o-nB9xk-Kl2O8yKTA1LQrQ/edit?usp=sharing
Change-Id: I7b421c6ed77d28ebab9b91a4d6fcb1e825997e6c
---
M src/kudu/tablet/compaction_policy-test.cc
M src/kudu/tablet/compaction_policy.cc
M src/kudu/tablet/rowset_info.cc
M src/kudu/tablet/rowset_info.h
4 files changed, 277 insertions(+), 37 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/69/11869/2
--
To view, visit http://gerrit.cloudera.org:8080/11869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: I7b421c6ed77d28ebab9b91a4d6fcb1e825997e6c
Gerrit-Change-Number: 11869
Gerrit-PatchSet: 2
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] [compaction] KUDU-1400: Improve rowset compaction policy to consider merging small DRSs
Posted by "Will Berkeley (Code Review)" <ge...@cloudera.org>.
Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11869 )
Change subject: [compaction] KUDU-1400: Improve rowset compaction policy to consider merging small DRSs
......................................................................
Patch Set 3:
The tablet history gc test failure looks like it was caused by this patch, but not a bug. Basically, the following happened:
0. The test is tuned to do maintenance ops constantly.
1. Rows are inserted. A flush happens while rows are still being inserted.
2. More rows are inserted. A second flush happens.
3. Wait for maintenance ops to flush some undos.
4. The clock is advanced to make the undos old enough to GC.
5. The two DRS are compacted because they are small. This GC's the undos as part of the process? Or it doesn't compact undos into redos because the AHM passed for the redos? I need to check how the undos might have done away.
6. While the compaction is working, all the undo gc ops run but can't gc the blocks of the rowsets being compacted.
7. The output rowset has no undos. The test times out waiting for its undos to be gc'd and for that to be shown in the undo op metrics.
--
To view, visit http://gerrit.cloudera.org:8080/11869
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I7b421c6ed77d28ebab9b91a4d6fcb1e825997e6c
Gerrit-Change-Number: 11869
Gerrit-PatchSet: 3
Gerrit-Owner: Will Berkeley <wd...@gmail.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Alexey Serbin <as...@cloudera.com>
Gerrit-Reviewer: Andrew Wong <aw...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins (120)
Gerrit-Reviewer: Tidy Bot (241)
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-Reviewer: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Fri, 02 Nov 2018 23:23:44 +0000
Gerrit-HasComments: No