[kudu-CR] [compaction] Cleanup of compaction policy code

["Will Berkeley (Code Review)" <ge...@cloudera.org>]

Will Berkeley has posted comments on this change. ( http://gerrit.cloudera.org:8080/11827 )

Change subject: [compaction] Cleanup of compaction policy code
......................................................................


Patch Set 3:

(10 comments)

http://gerrit.cloudera.org:8080/#/c/11827/3/src/kudu/tablet/compaction_policy-test.cc
File src/kudu/tablet/compaction_policy-test.cc:

http://gerrit.cloudera.org:8080/#/c/11827/3/src/kudu/tablet/compaction_policy-test.cc@58
PS3, Line 58: unordered_set<const RowSet*>
> nit: introduce a typedef?
Done


http://gerrit.cloudera.org:8080/#/c/11827/3/src/kudu/tablet/compaction_policy.h
File src/kudu/tablet/compaction_policy.h:

http://gerrit.cloudera.org:8080/#/c/11827/3/src/kudu/tablet/compaction_policy.h@62
PS3, Line 62: size
> nit: size in bytes ?
Done


http://gerrit.cloudera.org:8080/#/c/11827/3/src/kudu/tablet/compaction_policy.h@66
PS3, Line 66: std::numeric_limits<uint64_t>::max()
> Are you sure this is not a functional change?
Yes. For two reasons:

1. CompactionPolicy is never instantiated. Only its subclass BudgetedCompactionPolicy is, and it overrides this.
2. 1GB is larger than the default compaction budget of 128MB, so this value would not have been effective anyway.

Using uint64_t's max value is more consonant with the "no rolling" intention.


http://gerrit.cloudera.org:8080/#/c/11827/3/src/kudu/tablet/compaction_policy.h@99
PS3, Line 99:   void SetupKnapsackInput(const RowSetTree& tree,
> warning: function 'kudu::tablet::BudgetedCompactionPolicy::SetupKnapsackInp
Done


http://gerrit.cloudera.org:8080/#/c/11827/1/src/kudu/tablet/compaction_policy.h
File src/kudu/tablet/compaction_policy.h:

http://gerrit.cloudera.org:8080/#/c/11827/1/src/kudu/tablet/compaction_policy.h@a53
PS1, Line 53: 
> Is this done, or just a non-goal now? Regardless, it'd probably be helpful 
It's not important (at least right now) what the units are. It's just a double...there's no particular range for it. It tends to be in the range 0-1 for winning selections, but it could be much larger, and it is negative for lots of bad selections.

I don't want to refer to the doc here, again. The doc is already referred to a number of times throughout the compaction code.


http://gerrit.cloudera.org:8080/#/c/11827/1/src/kudu/tablet/compaction_policy.h@42
PS1, Line 42: ompactionPolicy() = default;
            : 
> not needed?
The compiler tells me I need it to implicitly initialize the base class in the subclass constructor and also to delete the base class when stored in a smart pointer.


http://gerrit.cloudera.org:8080/#/c/11827/1/src/kudu/tablet/compaction_policy.h@100
PS1, Line 100:                           std::vector<RowSetInfo>* asc_min_key,
> warning: function 'kudu::tablet::BudgetedCompactionPolicy::SetupKnapsackInp
Done


http://gerrit.cloudera.org:8080/#/c/11827/3/src/kudu/tablet/compaction_policy.cc
File src/kudu/tablet/compaction_policy.cc:

http://gerrit.cloudera.org:8080/#/c/11827/3/src/kudu/tablet/compaction_policy.cc@53
PS3, Line 53: advanced
> As I understand, this change moves the flag into the non-experimental domai
Whoops. That was not intentional. Good catch.


http://gerrit.cloudera.org:8080/#/c/11827/3/src/kudu/tablet/compaction_policy.cc@374
PS3, Line 374:  &
> style nit: stick the ampersand to the parameter type.
Done


http://gerrit.cloudera.org:8080/#/c/11827/3/src/kudu/tablet/tablet.cc
File src/kudu/tablet/tablet.cc:

http://gerrit.cloudera.org:8080/#/c/11827/3/src/kudu/tablet/tablet.cc@2317
PS3, Line 2317: *o
> nit: maybe, dereference the pointer just once and use the result reference 
Done



-- 
To view, visit http://gerrit.cloudera.org:8080/11827
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings

Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I955ec66a510f04fd869f7d969a643e4d0f7f415f
Gerrit-Change-Number: 11827
Gerrit-PatchSet: 3
Gerrit-Owner: Will Berkeley <wd...@gmail.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: Will Berkeley <wd...@gmail.com>
Gerrit-Comment-Date: Thu, 01 Nov 2018 16:51:40 +0000
Gerrit-HasComments: Yes