[kudu-CR] master: only use tablet reports to notify master of altered tablets
Adar Dembo has submitted this change and it was merged. Change subject: master: only use tablet reports to notify master of altered tablets .. master: only use tablet reports to notify master of altered tablets One or more alter table operations are not considered finished until the master is sure that all altered tablets have the newest schema version. Today the master is notified of these schema version changes via tablet report or via response to the AlterSchema() RPC, whichever comes first. Notification via AlterSchema() RPC response, however, is unsafe in a world with a single leader election lock, because taking the lock in an RPC response can lead to deadlocks. To avoid this, let's use tablet reports as the sole notification channel. The completion of an AlterSchema() transaction now marks the tablet as dirty; otherwise, there's no knowing exactly when the master would see the altered tablet in a tablet report. This notification change does not perfectly preserve existing semantics. For example, if AlterSchema() had returned TABLET_NOT_FOUND, the old code would make a "fake" call to CatalogManager::HandleTabletSchemaVersionReport() with the new schema version, potentially unblocking clients who are waiting on the alter table operation to complete. The new code can't do this; if tablet reports are the only notification mechanism, a tablet that isn't found will never report in. This can lead to client timeouts, though I'm unsure as to whether this is a bad thing or not. There are no new tests, but without the AlterSchema() change, many existing tests failed with timeouts in client-issued alter table operations. Change-Id: I09ff1560d0bd5cecefb08684ecf5c2c992d69d46 Reviewed-on: http://gerrit.cloudera.org:8080/3580 Tested-by: Kudu Jenkins Reviewed-by: Todd Lipcon --- M src/kudu/master/catalog_manager.cc M src/kudu/tablet/tablet_peer.h M src/kudu/tablet/transactions/alter_schema_transaction.cc 3 files changed, 13 insertions(+), 7 deletions(-) Approvals: Todd Lipcon: Looks good to me, approved Kudu Jenkins: Verified -- To view, visit http://gerrit.cloudera.org:8080/3580 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: merged Gerrit-Change-Id: I09ff1560d0bd5cecefb08684ecf5c2c992d69d46 Gerrit-PatchSet: 3 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] master: only use tablet reports to notify master of altered tablets
Adar Dembo has posted comments on this change. Change subject: master: only use tablet reports to notify master of altered tablets .. Patch Set 2: > would you mind looping alter_table-test a bit to make sure this > doesn't cause it to get flaky (or flakier as the case may be) I looped it 2000 times (once with KUDU_ALLOW_SLOW_TESTS=1, once without). All passed. -- To view, visit http://gerrit.cloudera.org:8080/3580 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I09ff1560d0bd5cecefb08684ecf5c2c992d69d46 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] master: only use tablet reports to notify master of altered tablets
Todd Lipcon has posted comments on this change. Change subject: master: only use tablet reports to notify master of altered tablets .. Patch Set 2: Code-Review+2 +2 assuming you did some loops -- To view, visit http://gerrit.cloudera.org:8080/3580 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I09ff1560d0bd5cecefb08684ecf5c2c992d69d46 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] master: only use tablet reports to notify master of altered tablets
Todd Lipcon has posted comments on this change. Change subject: master: only use tablet reports to notify master of altered tablets .. Patch Set 2: would you mind looping alter_table-test a bit to make sure this doesn't cause it to get flaky (or flakier as the case may be) -- To view, visit http://gerrit.cloudera.org:8080/3580 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I09ff1560d0bd5cecefb08684ecf5c2c992d69d46 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] master: only use tablet reports to notify master of altered tablets
Hello Kudu Jenkins, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/3580 to look at the new patch set (#2). Change subject: master: only use tablet reports to notify master of altered tablets .. master: only use tablet reports to notify master of altered tablets One or more alter table operations are not considered finished until the master is sure that all altered tablets have the newest schema version. Today the master is notified of these schema version changes via tablet report or via response to the AlterSchema() RPC, whichever comes first. Notification via AlterSchema() RPC response, however, is unsafe in a world with a single leader election lock, because taking the lock in an RPC response can lead to deadlocks. To avoid this, let's use tablet reports as the sole notification channel. The completion of an AlterSchema() transaction now marks the tablet as dirty; otherwise, there's no knowing exactly when the master would see the altered tablet in a tablet report. This notification change does not perfectly preserve existing semantics. For example, if AlterSchema() had returned TABLET_NOT_FOUND, the old code would make a "fake" call to CatalogManager::HandleTabletSchemaVersionReport() with the new schema version, potentially unblocking clients who are waiting on the alter table operation to complete. The new code can't do this; if tablet reports are the only notification mechanism, a tablet that isn't found will never report in. This can lead to client timeouts, though I'm unsure as to whether this is a bad thing or not. There are no new tests, but without the AlterSchema() change, many existing tests failed with timeouts in client-issued alter table operations. Change-Id: I09ff1560d0bd5cecefb08684ecf5c2c992d69d46 --- M src/kudu/master/catalog_manager.cc M src/kudu/tablet/tablet_peer.h M src/kudu/tablet/transactions/alter_schema_transaction.cc 3 files changed, 13 insertions(+), 7 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/80/3580/2 -- To view, visit http://gerrit.cloudera.org:8080/3580 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newpatchset Gerrit-Change-Id: I09ff1560d0bd5cecefb08684ecf5c2c992d69d46 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] master: only use tablet reports to notify master of altered tablets
Kudu Jenkins has posted comments on this change. Change subject: master: only use tablet reports to notify master of altered tablets .. Patch Set 2: Build Started http://104.196.14.100/job/kudu-gerrit/2210/ -- To view, visit http://gerrit.cloudera.org:8080/3580 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I09ff1560d0bd5cecefb08684ecf5c2c992d69d46 Gerrit-PatchSet: 2 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] master: only use tablet reports to notify master of altered tablets
Adar Dembo has posted comments on this change. Change subject: master: only use tablet reports to notify master of altered tablets .. Patch Set 1: > this change potentially changes the latency of alter table requests > right? this is likely not a bad thing , but you didn't need to > change timeouts on tests? did you loop this? Agreed about the latency change, but I didn't loop it because I don't expect the timing to change significantly. Marking a tablet as dirty also means the tserver will send a heartbeat immediately, so we're talking about a few more ms for the heartbeat RPC. -- To view, visit http://gerrit.cloudera.org:8080/3580 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I09ff1560d0bd5cecefb08684ecf5c2c992d69d46 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] master: only use tablet reports to notify master of altered tablets
David Ribeiro Alves has posted comments on this change. Change subject: master: only use tablet reports to notify master of altered tablets .. Patch Set 1: this change potentially changes the latency of alter table requests right? this is likely not a bad thing , but you didn't need to change timeouts on tests? did you loop this? -- To view, visit http://gerrit.cloudera.org:8080/3580 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I09ff1560d0bd5cecefb08684ecf5c2c992d69d46 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
[kudu-CR] master: only use tablet reports to notify master of altered tablets
Hello David Ribeiro Alves, Todd Lipcon, I'd like you to do a code review. Please visit http://gerrit.cloudera.org:8080/3580 to review the following change. Change subject: master: only use tablet reports to notify master of altered tablets .. master: only use tablet reports to notify master of altered tablets One or more alter table operations are not considered finished until the master is sure that all altered tablets have the newest schema version. Today the master is notified of these schema version changes via tablet report or via response to the AlterSchema() RPC, whichever comes first. Notification via AlterSchema() RPC response, however, is unsafe in a world with a single leader election lock, because taking the lock in an RPC response can lead to deadlocks. To avoid this, let's use tablet reports as the sole notification channel. The completion of an AlterSchema() transaction now marks the tablet as dirty; otherwise, there's no knowing exactly when the master would see the altered tablet in a tablet report. This notification change does not perfectly preserve existing semantics. For example, if AlterSchema() had returned TABLET_NOT_FOUND, the old code would make a "fake" call to CatalogManager::HandleTabletSchemaVersionReport() with the new schema version, potentially unblocking clients who are waiting on the alter table operation to complete. The new code can't do this; if tablet reports are the only notification mechanism, a tablet that isn't found will never report in. This can lead to client timeouts, though I'm unsure as to whether this is a bad thing or not. There are no new tests, but without the AlterSchema() change, many existing tests failed with timeouts in client-issued alter table operations. Change-Id: I09ff1560d0bd5cecefb08684ecf5c2c992d69d46 --- M src/kudu/master/catalog_manager.cc M src/kudu/tablet/tablet_peer.h M src/kudu/tablet/transactions/alter_schema_transaction.cc 3 files changed, 13 insertions(+), 7 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/80/3580/1 -- To view, visit http://gerrit.cloudera.org:8080/3580 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: newchange Gerrit-Change-Id: I09ff1560d0bd5cecefb08684ecf5c2c992d69d46 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Todd Lipcon
[kudu-CR] master: only use tablet reports to notify master of altered tablets
Kudu Jenkins has posted comments on this change. Change subject: master: only use tablet reports to notify master of altered tablets .. Patch Set 1: Build Started http://104.196.14.100/job/kudu-gerrit/2203/ -- To view, visit http://gerrit.cloudera.org:8080/3580 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-MessageType: comment Gerrit-Change-Id: I09ff1560d0bd5cecefb08684ecf5c2c992d69d46 Gerrit-PatchSet: 1 Gerrit-Project: kudu Gerrit-Branch: master Gerrit-Owner: Adar Dembo Gerrit-Reviewer: David Ribeiro Alves Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-HasComments: No
