Todd Lipcon 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 7: (5 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(1) << Substitute("Setting schema version $0 for next log segment $1", This should probably have the tablet ID in the log message, or maybe bump to VLOG(2) if we think it will be redundant/noisy with other alter-related logging 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: Requested nit: I think this message would be slightly more informative if it said "Skipping requested alter" or something so that the user knows what the result was, not just what the situation was 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. > Clarifying the behavior of writes: I don't see any real harm either way. The one advantage of including the result in the COMMIT message even in the "OK" state is that we'll be able to distinguish a successfully applied alter vs an alter that may have failed on a previous version that didnt have this fix http://gerrit.cloudera.org:8080/#/c/12462/6/src/kudu/tablet/tablet_bootstrap.cc@1465 PS6, Line 1465: DCHECK(op.has_failed_status()); just for future compatibility (in case we change our mind about the above) I think it's worth actually checking if it's a failed status before skipping http://gerrit.cloudera.org:8080/#/c/12462/6/src/kudu/tablet/tablet_bootstrap.cc@1482 PS6, Line 1482: if (tx_state.error()) { why isn't there a "!" here? -- 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: 7 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 18:19:01 +0000 Gerrit-HasComments: Yes
