Adar Dembo has posted comments on this change. Change subject: catalog_manager: don't log deleted tables/tablets at startup ......................................................................
Patch Set 2: (8 comments) > How confident are we that the Substitutes won't be more eagerly > evaluated than the << operations? We discussed this offline. I wrote a test that evaluated this code: VLOG(1) << Substitute("abcd $0", [&]() { CHECK(false); return string("12345"); }()); When run normally, it passed. When run with -v=2, it aborted. So it appears that glog is lazily evaluating the entirety of the expression after a VLOG. http://gerrit.cloudera.org:8080/#/c/7826/2//COMMIT_MSG Commit Message: PS2, Line 14: overriden > nit: overridden Done http://gerrit.cloudera.org:8080/#/c/7826/1/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: Line 1308: LOG(INFO) << Substitute("Servicing CreateTable request from $0: $1", > The previous version has a newline before the table descriptor. I've alway I did this to be consistent with AlterTable/DeleteTable, but I don't mind a newline for all three. Line 2357: VLOG(2) << Substitute("Received tablet report from $0: $1", > Might be good to put a leading newline on the tablet report here for the sa Done PS1, Line 3133: deleted > I think it was correct before Whoops. http://gerrit.cloudera.org:8080/#/c/7826/2/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: PS2, Line 359: VLOG(2) > interesting: for tables the debug log level is 1. Is this intentional to h That's the way the old code was; can't say for sure since I didn't write it (I don't think). But I'll put them at the same log level. PS2, Line 2616: WARNING > Maybe, it's worth set to up ERROR since this is an error condition and the Done PS2, Line 2951: ... > nit: drop the ellipsis? Done PS2, Line 2947: if (delay_millis <= 0) { : LOG(WARNING) << "Request timed out: " << description(); : MarkFailed(); : } else { : LOG(INFO) << Substitute("Scheduling retry of $0 with a delay of $1 ms (attempt = $2)...", : description(), delay_millis, attempt_); : master_->messenger()->ScheduleOnReactor( : boost::bind(&RetryingTSRpcTask::RunDelayedTask, this, _1), : MonoDelta::FromMilliseconds(delay_millis)); : return true; : } : return false; > nit: while you are at it, consider removing the extra-scope for better read Done -- To view, visit http://gerrit.cloudera.org:8080/7826 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I557ebfe9d3ce1663e7f7c189f0087bdd29a09306 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo <a...@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: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-HasComments: Yes