Alexey Serbin has posted comments on this change. Change subject: [catalog_manager] categorization of rw operation failures ......................................................................
Patch Set 10: (10 comments) http://gerrit.cloudera.org:8080/#/c/6170/10/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: Line 465: // If there is an error (e.g., we are not the leader) abort this task > With the additions below, this path is no longer aborting the task. I updated the message correspondingly. Line 477: bool error_is_not_leader; > Now we have two separate async callback codepaths using two separate method Good point. I originally started to use the same term comparison approach here, but then I became concerned with races in this case -- since this is not the elected-as-a-leader callback. After modifications on the ScopedLeaderSharedLock this should not happen. Line 482: string err_msg = "failed to refresh TSK: " + s.ToString() + ": "; > I'm still worried about the general chattiness of these INFO logs. They me Yep, that's why they are INFOs Line 487: LOG(INFO) << err_msg << "will try next time"; > This is an unexpected codepath, right? Should be at least a warning: Done Line 494: LOG(FATAL) << "TokenSigner has left with no valid key to use"; > This message doesn't include the failure. Consider: Done Line 768: LOG(INFO) << "Did not find CA certificate and key for Kudu IPKI, " > The way this is worded makes it sound like a failure, consider instead: I dropped this message and left the message in StoreCertAuthorityInfo Line 853: LOG(INFO) << "Successfully stored the newly generated certificate authority " > This information is already indicated by the log message online 768, consid I decided to drop that at line 768 and keep this one. Line 971: LOG(INFO) << kCaInitOpDescription << "..."; > This is mostly redundant with the log message on line 768, consider droppin I don't think this is not redundant -- the message on line 768 triggers only once when there is no CA record in the system table yet. However, this message reports on the initialization of CA component, which is a broader thing and happens on every elected-as-leader-callback. Line 3427: LOG(INFO) << "Saved newly generated TSK " << tsk->key_seq_num() > This is a useful log message, but I think the details about saving and syst Done http://gerrit.cloudera.org:8080/#/c/6170/10/src/kudu/master/sys_catalog-test.cc File src/kudu/master/sys_catalog-test.cc: Line 422: const Status& s = master_->catalog_manager()->sys_catalog()-> > nit: by value 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: 10 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
