[6/7] incubator-kudu git commit: master: only use tablet reports to notify master of altered tablets

[ad...@apache.org]

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 <to...@apache.org>


Project: http://git-wip-us.apache.org/repos/asf/incubator-kudu/repo
Commit: http://git-wip-us.apache.org/repos/asf/incubator-kudu/commit/fb41e96d
Tree: http://git-wip-us.apache.org/repos/asf/incubator-kudu/tree/fb41e96d
Diff: http://git-wip-us.apache.org/repos/asf/incubator-kudu/diff/fb41e96d

Branch: refs/heads/master
Commit: fb41e96d2405337696f76b5af532bd2770f839ad
Parents: b130c5a
Author: Adar Dembo <ad...@cloudera.com>
Authored: Wed Jul 6 15:01:06 2016 -0700
Committer: Adar Dembo <ad...@cloudera.com>
Committed: Thu Jul 7 18:32:40 2016 +0000

----------------------------------------------------------------------
 src/kudu/master/catalog_manager.cc                       |  6 +-----
 src/kudu/tablet/tablet_peer.h                            | 10 ++++++++--
 src/kudu/tablet/transactions/alter_schema_transaction.cc |  4 ++++
 3 files changed, 13 insertions(+), 7 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/incubator-kudu/blob/fb41e96d/src/kudu/master/catalog_manager.cc
----------------------------------------------------------------------
diff --git a/src/kudu/master/catalog_manager.cc b/src/kudu/master/catalog_manager.cc
index 0c4bacc..1073a83 100644
--- a/src/kudu/master/catalog_manager.cc
+++ b/src/kudu/master/catalog_manager.cc
@@ -2313,9 +2313,7 @@ class AsyncAlterTable : public RetryingTSRpcTask {
       VLOG(1) << "TS " << permanent_uuid() << ": alter complete on tablet " << tablet_->ToString();
     }
 
-    if (state() == kStateComplete) {
-      master_->catalog_manager()->HandleTabletSchemaVersionReport(tablet_.get(), schema_version_);
-    } else {
+    if (state() != kStateComplete) {
       VLOG(1) << "Still waiting for other tablets to finish ALTER";
     }
   }
@@ -2329,7 +2327,6 @@ class AsyncAlterTable : public RetryingTSRpcTask {
     req.set_new_table_name(l.data().pb.name());
     req.set_schema_version(l.data().pb.version());
     req.mutable_schema()->CopyFrom(l.data().pb.schema());
-    schema_version_ = l.data().pb.version();
 
     l.Unlock();
 
@@ -2341,7 +2338,6 @@ class AsyncAlterTable : public RetryingTSRpcTask {
     return true;
   }
 
-  uint32_t schema_version_;
   scoped_refptr<TabletInfo> tablet_;
   tserver::AlterSchemaResponsePB resp_;
 };

http://git-wip-us.apache.org/repos/asf/incubator-kudu/blob/fb41e96d/src/kudu/tablet/tablet_peer.h
----------------------------------------------------------------------
diff --git a/src/kudu/tablet/tablet_peer.h b/src/kudu/tablet/tablet_peer.h
index 20f03e3..e926f79 100644
--- a/src/kudu/tablet/tablet_peer.h
+++ b/src/kudu/tablet/tablet_peer.h
@@ -252,6 +252,11 @@ class TabletPeer : public RefCountedThreadSafe<TabletPeer>,
     return meta_;
   }
 
+  // Marks the tablet as dirty so that it's included in the next heartbeat.
+  void MarkTabletDirty(const std::string& reason) {
+    mark_dirty_clbk_.Run(reason);
+  }
+
  private:
   friend class RefCountedThreadSafe<TabletPeer>;
   friend class TabletPeerTest;
@@ -316,8 +321,9 @@ class TabletPeer : public RefCountedThreadSafe<TabletPeer>,
   scoped_refptr<log::LogAnchorRegistry> log_anchor_registry_;
 
   // Function to mark this TabletPeer's tablet as dirty in the TSTabletManager.
-  // This function must be called any time the cluster membership or cluster
-  // leadership changes.
+  //
+  // Must be called whenever cluster membership or leadership changes, or when
+  // the tablet's schema changes.
   Callback<void(const std::string& reason)> mark_dirty_clbk_;
 
   // List of maintenance operations for the tablet that need information that only the peer

http://git-wip-us.apache.org/repos/asf/incubator-kudu/blob/fb41e96d/src/kudu/tablet/transactions/alter_schema_transaction.cc
----------------------------------------------------------------------
diff --git a/src/kudu/tablet/transactions/alter_schema_transaction.cc b/src/kudu/tablet/transactions/alter_schema_transaction.cc
index 4c81530..4ee5f0f 100644
--- a/src/kudu/tablet/transactions/alter_schema_transaction.cc
+++ b/src/kudu/tablet/transactions/alter_schema_transaction.cc
@@ -112,6 +112,10 @@ Status AlterSchemaTransaction::Apply(gscoped_ptr<CommitMsg>* commit_msg) {
     ->SetSchemaForNextLogSegment(*DCHECK_NOTNULL(state_->schema()),
                                                  state_->schema_version());
 
+  // Altered tablets should be included in the next tserver heartbeat so that
+  // clients waiting on IsAlterTableDone() are unblocked promptly.
+  state_->tablet_peer()->MarkTabletDirty("Alter schema finished");
+
   commit_msg->reset(new CommitMsg());
   (*commit_msg)->set_op_type(ALTER_SCHEMA_OP);
   return Status::OK();