Todd Lipcon has posted comments on this change. ( http://gerrit.cloudera.org:8080/9052 )
Change subject: KUDU-1927: no half-baked responses on ConnectoToMaster ...................................................................... Patch Set 3: (6 comments) http://gerrit.cloudera.org:8080/#/c/9052/3//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/9052/3//COMMIT_MSG@7 PS3, Line 7: KUDU-1927: no half-baked responses on ConnectoToMaster I'm a bit confused on this -- the JIRA linked here is marked as "wont fix" back in June, but here you're patching it. Can you explain in the commit message why it is we need this patch? And perhaps we need to re-open the JIRA explaining why our previous "wont fix" logic was not right? http://gerrit.cloudera.org:8080/#/c/9052/3/src/kudu/integration-tests/master_cert_authority-itest.cc File src/kudu/integration-tests/master_cert_authority-itest.cc: http://gerrit.cloudera.org:8080/#/c/9052/3/src/kudu/integration-tests/master_cert_authority-itest.cc@416 PS3, Line 416: // Add master-only flags. : const vector<string> master_flags = { : Substitute("--catalog_manager_inject_latency_load_ca_info_ms=$0", latency_ms_), : }; : copy(master_flags.begin(), master_flags.end(), : back_inserter(cluster_opts_.extra_master_flags)); isn't this simpler to just do cluster_opts_.extra_master_flags.push_back(...) instead of making and copying a vector? http://gerrit.cloudera.org:8080/#/c/9052/3/src/kudu/integration-tests/master_cert_authority-itest.cc@438 PS3, Line 438: const int hb_interval_ms = 50; does this always eventually converge to elect even in tsan? http://gerrit.cloudera.org:8080/#/c/9052/3/src/kudu/master/master_cert_authority.h File src/kudu/master/master_cert_authority.h: http://gerrit.cloudera.org:8080/#/c/9052/3/src/kudu/master/master_cert_authority.h@52 PS3, Line 52: parallel other "parallel with other" Why did you move this away from the "Init()" method itself? It seemed to make more sense there instead of at the class-level. I think it's also better to keep the comment scoped to this own class's behavior rather than talking about the overall design of what might use the class. In other words, it's enough to just say "NOTE: Init() is not thread-safe must be called before any other methods." http://gerrit.cloudera.org:8080/#/c/9052/3/src/kudu/master/master_cert_authority.h@94 PS3, Line 94: // Access the CA certificate object. doesn't really add much http://gerrit.cloudera.org:8080/#/c/9052/3/src/kudu/master/master_cert_authority.h@97 PS3, Line 97: // Whether the object is initialized. worth noting here something like "requires external synchronization against calls of Init()" or something -- To view, visit http://gerrit.cloudera.org:8080/9052 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ib9c906863f5f0e1995041281b122135e1b2cd3a4 Gerrit-Change-Number: 9052 Gerrit-PatchSet: 3 Gerrit-Owner: Alexey Serbin <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Tue, 23 Jan 2018 00:16:56 +0000 Gerrit-HasComments: Yes
