Dan Burkert has posted comments on this change.

Change subject: KUDU-1474: single to multi-master deployment migration

Patch Set 4:


File src/kudu/integration-tests/master_migration-itest.cc:

Line 44: using std::string;
using std::shared_ptr;

Line 69:       cluster_.reset();
This reset is redundant, right?

Line 80:                    const std::string& table_name) {

Line 109:   return scanner.Open();
This seems kind of fragile (relying on Open to actually talk to a tserver is 
way outside the purview of this test).  I think a more robust (and simpler) way 
would be to just call ASSERT_EQ(0, CountTableRows())

Line 114:   const char* kTableName = "test";

Line 164:       args.push_back(Substitute("$0:$1", m.first, 
question: this is implying that both UUID and address are added to the raft 
config?  Is that true for tablets as well as masters?

PS4, Line 173: it cannot replicate to them
because they aren't up, or because they aren't caught up?

File src/kudu/master/master.cc:

PS4, Line 117: remote_bootstrap_service
should be updated to the new name, right?

File src/kudu/master/sys_catalog.h:

PS4, Line 67: static constexpr const char*
static const string

File src/kudu/tools/tool_action.cc:

PS4, Line 53:  
extra space?

PS4, Line 58: BuildLeafActionHelpString
Where does it actually print out the arguments required for the leaf action?

File src/kudu/tools/tool_action.h:

PS4, Line 51: char*
Any reason to prefer raw pointers to string here?

Line 69: std::string BuildActionChainString(std::vector<Action> chain);
It seems like at least some, if not all, of these could take the chain by const 

PS4, Line 91: std::list
Maybe it would indicate intent more if you used a std::deque here, unless I 
missed something and you do actually need random removal.

File src/kudu/tools/tool_action_fs.cc:

Line 56: Action BuildFsAction() {
I mean this terse syntax is great and all, but I think what we really need is 
some kind of XML layout and documentation engine/generator to really give our 
CLI some pop.

Line 57:   Action fs_format;
I think you can use struct literal syntax here to clean this up, something like:

Action fs_format = {
  "Format a new Kudu filesystem",

Although you may prefer the explicitness.

File src/kudu/tools/tool_action_tablet.cc:

PS4, Line 107: .pre_rewrite
maybe add a timestamp?

PS4, Line 111: gscoped_ptr

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

Gerrit-MessageType: comment
Gerrit-Change-Id: I89c741381ced3731736228cd07fe85106ae72541
Gerrit-PatchSet: 4
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <a...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <d...@cloudera.com>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Mike Percy <mpe...@apache.org>
Gerrit-Reviewer: Todd Lipcon <t...@apache.org>
Gerrit-HasComments: Yes

Reply via email to