David Ribeiro Alves has posted comments on this change. Change subject: [catalog_manager] categorization of rw operation failures ......................................................................
Patch Set 28: (21 comments) http://gerrit.cloudera.org:8080/#/c/6170/28//COMMIT_MSG Commit Message: PS28, Line 11: extra space PS28, Line 17: ElectedAsLeaderCb "...execution of the ElectedAsLeaderCb" PS28, Line 19: at the same term of the : catalog leadership within the same term of catalog leadership PS28, Line 21: possible a possible http://gerrit.cloudera.org:8080/#/c/6170/28/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: PS28, Line 491: else if (signer->IsCurrentKeyValid()) { : LOG(WARNING) << err_msg << "will try again next cycle"; didn't you mention in the commit message that this would crash the server? seems like you have an exception if the key is still valid. That makes sense, but could you update the commit msg? (or am I missing something?? PS28, Line 497: o avoid possible : // inconsistency, let's crash the process. add that another master will take over as leader. PS28, Line 764: // Once successfully loaded, the CA information is supposed to be valid: : // the leader master should not run without CA certificate. not sure how this comment is relevant here. maybe move to the method's header? PS28, Line 775: information what information? PS28, Line 776: This is to protect against : // a leadership change in the middle. "This protects against a leadership change between the generation and storage of ..." PS28, Line 794: consensus nit" remove "consensus" PS28, Line 796: consensus same PS28, Line 798: occures occurs PS28, Line 804: A failure is the only possible outcome of the attempted write : // operation if the catalog manager is not the the leader of the : // the system tablet's consensus. maybe just: An error message is logged and the failure is ignored. PS28, Line 809: opeartion operation PS28, Line 811: This is when the former in-the-middle leader has : // not succeeded in writing the CA info it generated maybe just "Success. The master completes the initialization process and proceeds to serve client requests." Failure. The master can't complete writing its CA info (likely the previous leader master has written one). Since the CA info record // has pre-defined identifier, it's impossible to have more // than one CA info record in the system table. This is due to // the {record_id, record_type} uniqueness constraint. PS28, Line 836: persisted is persisted PS28, Line 844: role is lost at this moment maybe just: If leadership was lost PS28, Line 844: It typo PS28, Line 848: that it PS28, Line 4125: IllegalState There's actually Status::Uninitialized(), same below (and thanks for fixing this) http://gerrit.cloudera.org:8080/#/c/6170/28/src/kudu/master/master_service.cc File src/kudu/master/master_service.cc: PS28, Line 200: In regular mode what's regular mode. maybe just mention the flag as an exception and leave the rest? -- To view, visit http://gerrit.cloudera.org:8080/6170 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I826826049e3c08a6c8345949690cbbedaea32ff8 Gerrit-PatchSet: 28 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Alexey Serbin <aser...@cloudera.com> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: David Ribeiro Alves <davidral...@gmail.com> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes
