Dan Burkert 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. Line 477: bool error_is_not_leader; Now we have two separate async callback codepaths using two separate methods to determine if leadership has been lost. Could you use the term comparison method here as well? Line 482: string err_msg = "failed to refresh TSK: " + s.ToString() + ": "; I'm still worried about the general chattiness of these INFO logs. They mention failures, but they are normal and expected, and don't require operator action. Line 487: LOG(INFO) << err_msg << "will try next time"; This is an unexpected codepath, right? Should be at least a warning: LOG(WARNING) << "Failed to generate TSK: " << s.ToString(); Line 494: LOG(FATAL) << "TokenSigner has left with no valid key to use"; This message doesn't include the failure. Consider: LOG(FATAL) << "Failed to generate TSK: " << s.ToString(); 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: "Generating a new IPKI CA certificate and key" Line 853: LOG(INFO) << "Successfully stored the newly generated certificate authority " This information is already indicated by the log message online 768, consider dropping this or reducing to VLOG Line 971: LOG(INFO) << kCaInitOpDescription << "..."; This is mostly redundant with the log message on line 768, consider dropping this or reducing to VLOG. 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 system table are not helpful. Consider changing it to: LOG(INFO) << "Generated new TSK " << tsk->key_seq_num(); 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 -- 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
