Mike Percy has posted comments on this change. ( http://gerrit.cloudera.org:8080/8090 )
Change subject: KUDU-1125: issue one catalog write per tablet report ...................................................................... Patch Set 7: (9 comments) This is a big improvement, Adar. http://gerrit.cloudera.org:8080/#/c/8090/6/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/8090/6/src/kudu/master/catalog_manager.cc@3106 PS6, Line 3106: "requestor", rpc->requestor_string(), > would be nice to also use TRACE_COUNTER_INCREMENT("reported_tablets", updat Todd, I'm not sure how your example can help measure latency. Did you mean to suggest using something like TRACE_COUNTER_SCOPE_LATENCY_US() ? http://gerrit.cloudera.org:8080/#/c/8090/6/src/kudu/master/catalog_manager.cc@3113 PS6, Line 3113: TODO(todd) per git blame http://gerrit.cloudera.org:8080/#/c/8090/7/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/8090/7/src/kudu/master/catalog_manager.cc@3152 PS7, Line 3152: acqurie acquire http://gerrit.cloudera.org:8080/#/c/8090/7/src/kudu/master/catalog_manager.cc@3173 PS7, Line 3173: // reasonable to ignore it and wait for an operator fix the situation. The end of this comment only makes sense after going to the flag definition and seeing that it defaults to false. I wonder, if we are going to default it to false, if we shouldn't just remove it and this by-default-disabled logic altogether and simply emit the warning on L3181. When was the last time we enabled this? I actually thought the default behavior of the master was to delete unknown tablets. http://gerrit.cloudera.org:8080/#/c/8090/7/src/kudu/master/catalog_manager.cc@3236 PS7, Line 3236: report.has_consensus_state() I think this tertiary if condition needs to be: (report.has_consensus_state() && report.consensus_state().committed_config().has_opid_index()) because of the issue brought up in the comment on L3269. http://gerrit.cloudera.org:8080/#/c/8090/7/src/kudu/master/catalog_manager.cc@3291 PS7, Line 3291: Should it? Interesting question. I think it technically should, because the purpose of disregarding the non-member leaders is to wait until a leader is part of the committed config before publishing its existence to clients. OTOH I can't easily come up with a series of events where this would be a problem because to change the config there has to be an initial leader that coordinates the config change, and this logic is about waiting for that first leader when creating a new tablet from scratch for the first time. http://gerrit.cloudera.org:8080/#/c/8090/7/src/kudu/master/catalog_manager.cc@3319 PS7, Line 3319: config cstate; a config doesn't really have a term http://gerrit.cloudera.org:8080/#/c/8090/7/src/kudu/master/catalog_manager.cc@3320 PS7, Line 3320: config cstate http://gerrit.cloudera.org:8080/#/c/8090/7/src/kudu/master/catalog_manager.cc@3452 PS7, Line 3452: as in -- To view, visit http://gerrit.cloudera.org:8080/8090 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie6f5cf0e4b1cd1160b3b310d89c6dbf3dd62e43b Gerrit-Change-Number: 8090 Gerrit-PatchSet: 7 Gerrit-Owner: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Dan Burkert <danburk...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <mpe...@apache.org> Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Tue, 03 Oct 2017 02:55:41 +0000 Gerrit-HasComments: Yes
