Dan Burkert has posted comments on this change. Change subject: [catalog_manager] categorization of rw operation failures ......................................................................
Patch Set 8: (10 comments) http://gerrit.cloudera.org:8080/#/c/6170/8//COMMIT_MSG Commit Message: PS8, Line 10: d/write operation failures happened while executing the leader : post-election callback sentence fragment PS8, Line 14: / Make it explicit whether this means 'and' or 'or'. http://gerrit.cloudera.org:8080/#/c/6170/8/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: Line 463: const Status& s = catalog_manager_->ProcessPendingAssignments(to_process); Would you mind making this an owned Status here and below? I find the by-value return stored as reference pattern to be very unintuitive, and it doesn't match what's actually happening. PS8, Line 763: found is found PS8, Line 770: the master PS8, Line 774: already : // contain CA certificate information written by the new leader master contain the CA certificate information when this master is re-elected next. Line 790: return s; I'm guessing this is an unnecessary copy because the status is kept as a reference. If it weren't a reference this is pretty much guaranteed NRVO. PS8, Line 911: const Consensus& consensus this could just be captured. Line 914: const int64_t term_pre = Why not use 'term'? Line 938: LOG(WARNING) << op_description << " failed; " This is logged at INFO level on line 899, why WARNING here? -- 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: 8 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: Tidy Bot Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-HasComments: Yes
