Alexey Serbin 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 Done PS28, Line 17: ElectedAsLeaderCb > "...execution of the ElectedAsLeaderCb" Done PS28, Line 19: at the same term of the : catalog leadership > within the same term of catalog leadership Done PS28, Line 21: possible > a possible Done 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? Right, the commit message simplifies the matter a little bit. I updated the commit message accordingly. PS28, Line 497: o avoid possible : // inconsistency, let's crash the process. > add that another master will take over as leader. Done 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 head ok, I removed the comment. PS28, Line 775: information > what information? newly generated IPKI CA information: the CA private key and the CA certificate. PS28, Line 776: This is to protect against : // a leadership change in the middle. > "This protects against a leadership change between the generation and stora Done PS28, Line 794: consensus > nit" remove "consensus" Done PS28, Line 796: consensus > same Done PS28, Line 798: occures > occurs Done 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. Done PS28, Line 809: opeartion > operation Done 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 pr Done PS28, Line 836: persisted > is persisted Done PS28, Line 844: role is lost at this moment > maybe just: If leadership was lost Done PS28, Line 844: It > typo Done PS28, Line 848: that > it Done PS28, Line 4125: IllegalState > There's actually Status::Uninitialized(), same below (and thanks for fixing Done 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 Done -- 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 <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: David Ribeiro Alves <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
