Adar Dembo 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 pro I copied this part of the comment verbatim since I don't pretend to understand the nuances of catch-up, but I'll use your wording. http://gerrit.cloudera.org:8080/#/c/8090/5/src/kudu/master/catalog_manager.cc@3274 PS5, Line 3274: // of the committed config, > s/,/. I'm not sure. I think it might be possible if the tablet report is coming from a tombstone rather than a live replica. Tombstones might have a very stale notion of who is in the replication group as I don't believe they receive config changes. 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? I left it in because of the "background thread" part, but I agree that this change is more than enough to merit. 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? Yes, your edit sums it up well. I'll reword this. 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 Hmm. Given long enough elections, every term change will first be reported as having no leader, and only later will report a leader. What's the use of the master persisting that intermediate state? AFAICT the master only cares about persisting leadership so that it can use that information immediately after a reboot, before all tablets report in. Even that is a sketchy argument; leadership could probably be in-memory only and no one would be the wiser. 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)? If you think a third level of nesting is more clear, sure I'll do that. 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? Yes, and it uses a CAS on cstate's opid_index so that if the index changes, the RPC is discarded. I'll update the comment to reflect that. 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? The old code did that, so I continued doing it here. 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 Agreed. 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 tab While "table before tablets when acquiring and the reverse when releasing" rule is a requirement to avoid deadlocks for WRITE locks, it doesn't matter for READ locks. We could hold the table locks for longer, but it just creates more contention w.r.t. table writes than needed. If it makes you feel better, the old code used this ordering too (albeit on a single table/tablet at a time). 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. Done 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 tab DeleteReplica() for an unknown 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 <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: Fri, 22 Sep 2017 22:24:47 +0000 Gerrit-HasComments: Yes
