Alexey Serbin 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: (7 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" Original intention was to fix not exactly KUDU-1927, but some other (at least by understanding) issue. However, during the review, Adar raised a question about KUDU-1927, so I decided to address that as well. I spotted that issue while working on AuthTokenIssuingTest.ChannelConfidentiality. The issue I was trying to fix was: the master has its role as leader according to the consensus information, but CatalogManager::ScopedLeaderSharedLock::leader_status().ok() returning 'false' as at https://github.com/apache/kudu/blob/master/src/kudu/master/master_service.cc#L473 By my understanding, KUDU-1927 was more about CatalogManager::ScopedLeaderSharedLock::leader_status().ok() returning 'true', but the CA cert hasn't been generated yet or the token signer doesn't have valid signing key. That's why I didn't re-open the JIRA issue. I'll do it now, though. 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(.. Yes, I think it. It seems the current code just came from ConnectToClusterBaseTest's constructor. 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? It does not, and that's the reason behind flakiness in Jenkins pre-commit builds. 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" Ah, it seems I started some modifications on the class, and then I rolled them back and updated the comment. But I should have moved it back. Done. http://gerrit.cloudera.org:8080/#/c/9052/3/src/kudu/master/master_cert_authority.h@82 PS3, Line 82: Status SignServerCSR(const std::string& csr_der, const rpc::RemoteUser& caller, > warning: function 'kudu::master::MasterCertAuthority::SignServerCSR' has a Done 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 That's fair: I removed the comment. 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 I thought it would be enough to have that NOTE in the class comment. However, I think it's nice to have a comment as you suggested: done. -- 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 01:58:03 +0000 Gerrit-HasComments: Yes
