Andrew Wong has posted comments on this change. ( http://gerrit.cloudera.org:8080/12462 )
Change subject: KUDU-2690: don't roll log schema on failed alter ...................................................................... Patch Set 8: (7 comments) http://gerrit.cloudera.org:8080/#/c/12462/6/src/kudu/consensus/log.cc File src/kudu/consensus/log.cc: http://gerrit.cloudera.org:8080/#/c/12462/6/src/kudu/consensus/log.cc@936 PS6, Line 936: VLOG_WITH_PREFIX(2) << Substitute("Setting schema version $0 for next log segment $1", > This should probably have the tablet ID in the log message, or maybe bump t Done http://gerrit.cloudera.org:8080/#/c/12462/6/src/kudu/tablet/tablet.cc File src/kudu/tablet/tablet.cc: http://gerrit.cloudera.org:8080/#/c/12462/6/src/kudu/tablet/tablet.cc@1262 PS6, Line 1262: Skipping > nit: I think this message would be slightly more informative if it said "Sk Done http://gerrit.cloudera.org:8080/#/c/12462/6/src/kudu/tablet/tablet_bootstrap.cc File src/kudu/tablet/tablet_bootstrap.cc: http://gerrit.cloudera.org:8080/#/c/12462/6/src/kudu/tablet/tablet_bootstrap.cc@1458 PS6, Line 1458: // 3. The commit message contains a 'result', which should only happen if the : // alter resulted in a failure. Exit out without attempting the alter. > I don't see any real harm either way. The one advantage of including the re Yeah, the effect of it seems pretty targeted: it only makes a difference upon upgrade, when there hasn't yet been enough activity to GC the legacy segments. I'll leave it for now, but revert the DCHECK for forward-compatibility. http://gerrit.cloudera.org:8080/#/c/12462/6/src/kudu/tablet/tablet_bootstrap.cc@1458 PS6, Line 1458: // 3. The commit message contains a 'result', which should only happen if the : // alter resulted in a failure. Exit out without attempting the alter. > I don't see any real harm either way. The one advantage of including the re Right, the benefit would only be in cases where we have a server that's upgraded, and Kudu hasn't written enough to GC the old segments. On the one hand, you'd be able to figure that out from the bootstrap logging, but thinking back on the most recent WAL-related issue, that marginal chance for higher QOL is maybe worth it? http://gerrit.cloudera.org:8080/#/c/12462/6/src/kudu/tablet/tablet_bootstrap.cc@1458 PS6, Line 1458: // 3. The commit message contains a 'result', which should only happen if the : // alter resulted in a failure. Exit out without attempting the alter. > I don't see any real harm either way. The one advantage of including the re Yeah. In most cases, we _should_ be able to determine that based on the Kudu version, but it's certainly possible that numerous rewrites of old logs could keep around result-less alter failures. As someone looking _just_ through tooling it might be nice, though the same info could be found in the bootstrapping logs. http://gerrit.cloudera.org:8080/#/c/12462/6/src/kudu/tablet/tablet_bootstrap.cc@1465 PS6, Line 1465: DCHECK_EQ(1, commit_msg.result().ops_size()); > just for future compatibility (in case we change our mind about the above) Done http://gerrit.cloudera.org:8080/#/c/12462/6/src/kudu/tablet/tablet_bootstrap.cc@1482 PS6, Line 1482: // Apply the alter schema to the tablet > why isn't there a "!" here? Ugh.. -- To view, visit http://gerrit.cloudera.org:8080/12462 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Id761851741297e29a4666bec0c34fc4f7285f715 Gerrit-Change-Number: 12462 Gerrit-PatchSet: 8 Gerrit-Owner: Andrew Wong <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Andrew Wong <[email protected]> Gerrit-Reviewer: Kudu Jenkins (120) Gerrit-Reviewer: Tidy Bot (241) Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Wed, 20 Feb 2019 21:00:08 +0000 Gerrit-HasComments: Yes
