Dan Burkert 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 5: (12 comments) http://gerrit.cloudera.org:8080/#/c/8090/5/src/kudu/master/catalog_manager.cc File src/kudu/master/catalog_manager.cc: http://gerrit.cloudera.org:8080/#/c/8090/5/src/kudu/master/catalog_manager.cc@3227 PS5, Line 3227: just been added to a pending config and are in the process of catching : // up to the log entry where they were added to the config Should this say 'just been added to the committed config and are in the process of bootstrapping'? http://gerrit.cloudera.org:8080/#/c/8090/5/src/kudu/master/catalog_manager.cc@3274 PS5, Line 3274: // of the committed config, s/,/. For my own edification, do you know off the top of your head if this can this happen in normal circumstances? I would not expect the tserver to think a tserver was a leader who wasn't in the config. http://gerrit.cloudera.org:8080/#/c/8090/5/src/kudu/master/catalog_manager.cc@3294 PS5, Line 3294: // TODO(matteo): we could batch the IO onto a background thread, or at I thought this TODO is what the commit is addressing? http://gerrit.cloudera.org:8080/#/c/8090/5/src/kudu/master/catalog_manager.cc@3303 PS5, Line 3303: // - There was an opid_index change. For my own edification, why do we update the on-disk cstate on opid change? Doesn't that happen for every write batch (i.e. a lot)? I would have expected it to be updated on term change || new knowledge of leader || config change. Edit: going to leave my original question because it shows what I think is a natural question for someone new to this code. However, I think the code _is_ doing what I originally expected, but the comment could be more clear. The cstate isn't changing for every opid increment; it's changing if the committed config changes (the config's opid_index just happens to be how this is recognized). http://gerrit.cloudera.org:8080/#/c/8090/5/src/kudu/master/catalog_manager.cc@3309 PS5, Line 3309: cstate.current_term() > prev_cstate.current_term() I'm would have expected that we'd update the on-disk cstate for every term change, even if we don't know the leader of the new term. http://gerrit.cloudera.org:8080/#/c/8090/5/src/kudu/master/catalog_manager.cc@3311 PS5, Line 3311: 7e Shouldn't this be 7d(i)? http://gerrit.cloudera.org:8080/#/c/8090/5/src/kudu/master/catalog_manager.cc@3370 PS5, Line 3370: // 7h. Add a server to the config if it is under-replicated. Is this idempotent? http://gerrit.cloudera.org:8080/#/c/8090/5/src/kudu/master/catalog_manager.cc@3386 PS5, Line 3386: report.schema_version()); Still send the AsyncAlterTable? http://gerrit.cloudera.org:8080/#/c/8090/5/src/kudu/master/catalog_manager.cc@3396 PS5, Line 3396: AsyncAlterTable Would have expected this to be called AsyncAlterTablet, but I guess that's probably outside the scope here. http://gerrit.cloudera.org:8080/#/c/8090/5/src/kudu/master/catalog_manager.cc@3408 PS5, Line 3408: tables_lock.Unlock(); Makes me nervous that this acquires tables before tablets, and releases tables before tablets as well. http://gerrit.cloudera.org:8080/#/c/8090/5/src/kudu/master/catalog_manager.cc@3418 PS5, Line 3418: LOG(ERROR) << Substitute("Error updating tablets: $0. Tablet report was: $1", Include the source tserver ID. http://gerrit.cloudera.org:8080/#/c/8090/5/src/kudu/master/catalog_manager.cc@3448 PS5, Line 3448: // response to a tablet report. Which RPC types does this apply to? AFAICT they all have an associated tablet. -- 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: 5 Gerrit-Owner: Adar Dembo <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Dan Burkert <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Mike Percy <[email protected]> Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Thu, 21 Sep 2017 22:44:09 +0000 Gerrit-HasComments: Yes
