Will Berkeley has posted comments on this change. Change subject: Use the same replica selection when adding a server as table creation ......................................................................
Patch Set 1: (6 comments) Want some feedback from Adar before submitting the next PS http://gerrit.cloudera.org:8080/#/c/7143/1/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: Line 3182: ThreadSafeRandom& rng) { > warning: non-const reference parameter 'rng', make it const or use a pointe Done Line 3208: } else if (load_b < load_a) { > warning: do not use 'else' after 'return' [readability-else-after-return] Done Line 3221: ThreadSafeRandom& rng) { > warning: non-const reference parameter 'rng', make it const or use a pointe Done Line 3248: vector<shared_ptr<TSDescriptor> > two_choices; > nit: in c++11 using >> is no longer a compiler parse error so no need for t Done Line 3255: if (two_choices.size() == 1) { > Why remove the CHECK from L3901 in the previous version of the file? The DCHECK asserted that, if there's not 2 choices, there's 1 choice. However, this function is now used in the add replica case as well as the create table case, and it's possible in the former that there's 2, 1, or 0 choices. The equivalent DCHECK for the create table case is now just after the call to SelectReplica() in SelectReplicas() ~L3906. Line 3352: rng_(GetRandomSeed32()) { > Can we reuse the catalog manager's ThreadSafeRandom instead of making a new What's your advice on how to do that? If you copy it then that would copy the state and potentially all the tasks spawned at about the same time would pick the same server. Should AsyncAddServer task hold a bare pointer to catalog manager's rng_? It seems safe since the catalog manager waits for its tasks to be aborted/destroyed before it shuts down itself. -- To view, visit http://gerrit.cloudera.org:8080/7143 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I199e7a59c2c7832e7a87842b357ba3aa29e34685 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Will Berkeley <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Will Berkeley <[email protected]> Gerrit-HasComments: Yes
