kudu-commits mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From t...@apache.org
Subject incubator-kudu git commit: client: Some simplifying cleanups and renames
Date Mon, 25 Jan 2016 04:06:04 GMT
Repository: incubator-kudu
Updated Branches:
  refs/heads/master 0d891c1ba -> a10e3739a


client: Some simplifying cleanups and renames

This patch removes the capability to look up a tablet in the cache by id
via a public API. That API was not actually required by any callers, and
there was a sketchy CHECK in there that enforced a tablet_id hit in the
cache, meaning there were sharp edges that had to be avoided.

Additionally, this patch renames RefreshProxy -> InitProxy and related
functions and removes the "force" bool that was never used by any
callers. There isn't really a reason to "refresh" a proxy unless somehow
the IP address had changed, which we don't currently support. When that
support gets added, the implementer can add whatever APIs make sense at
that time.

Change-Id: I3e825848c6edae69ae5a13e52eaf1e8b9dacb130
Reviewed-on: http://gerrit.cloudera.org:8080/1882
Reviewed-by: Todd Lipcon <todd@apache.org>
Tested-by: Kudu Jenkins


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

Branch: refs/heads/master
Commit: a10e3739a0e1b75289b49f31e094a095e2fd7bea
Parents: 0d891c1
Author: Mike Percy <mpercy@apache.org>
Authored: Fri Jan 22 13:31:57 2016 -0800
Committer: Todd Lipcon <todd@apache.org>
Committed: Mon Jan 25 04:05:46 2016 +0000

----------------------------------------------------------------------
 src/kudu/client/batcher.cc          | 13 ++++++-------
 src/kudu/client/client-internal.cc  | 11 ++++-------
 src/kudu/client/client-internal.h   |  4 ++--
 src/kudu/client/client-test.cc      | 14 +++++++-------
 src/kudu/client/meta_cache.cc       | 12 ++----------
 src/kudu/client/meta_cache.h        | 17 +++++------------
 src/kudu/client/scanner-internal.cc |  2 +-
 7 files changed, 27 insertions(+), 46 deletions(-)
----------------------------------------------------------------------


http://git-wip-us.apache.org/repos/asf/incubator-kudu/blob/a10e3739/src/kudu/client/batcher.cc
----------------------------------------------------------------------
diff --git a/src/kudu/client/batcher.cc b/src/kudu/client/batcher.cc
index 69a683c..f5ad0bb 100644
--- a/src/kudu/client/batcher.cc
+++ b/src/kudu/client/batcher.cc
@@ -203,9 +203,9 @@ class WriteRpc : public Rpc {
   // the rpc after a short delay.
   void LookupTabletCb(const Status& status);
 
-  // Called when we finish refreshing a TS proxy. Sends the RPC, provided
-  // there was no error.
-  void RefreshTSProxyCb(const Status& status);
+  // Called when we finish initializing a TS proxy.
+  // Sends the RPC, provided there was no error.
+  void InitTSProxyCb(const Status& status);
 
   // Marks all replicas on current_ts_ as failed and retries the write on a
   // new replica.
@@ -381,9 +381,8 @@ void WriteRpc::SendRpc() {
   }
 
   // Make sure we have a working proxy before sending out the RPC.
-  current_ts_->RefreshProxy(batcher_->client_,
-                            Bind(&WriteRpc::RefreshTSProxyCb, Unretained(this)),
-                            false);
+  current_ts_->InitProxy(batcher_->client_,
+                         Bind(&WriteRpc::InitTSProxyCb, Unretained(this)));
 }
 
 string WriteRpc::ToString() const {
@@ -402,7 +401,7 @@ void WriteRpc::LookupTabletCb(const Status& status) {
   mutable_retrier()->DelayedRetry(this, status);
 }
 
-void WriteRpc::RefreshTSProxyCb(const Status& status) {
+void WriteRpc::InitTSProxyCb(const Status& status) {
   // Fail to a replica in the event of a DNS resolution failure.
   if (!status.ok()) {
     FailToNewReplica(status);

http://git-wip-us.apache.org/repos/asf/incubator-kudu/blob/a10e3739/src/kudu/client/client-internal.cc
----------------------------------------------------------------------
diff --git a/src/kudu/client/client-internal.cc b/src/kudu/client/client-internal.cc
index bc56abd..7c03aaf 100644
--- a/src/kudu/client/client-internal.cc
+++ b/src/kudu/client/client-internal.cc
@@ -295,16 +295,13 @@ RemoteTabletServer* KuduClient::Data::SelectTServer(const scoped_refptr<RemoteTa
 }
 
 Status KuduClient::Data::GetTabletServer(KuduClient* client,
-                                         const string& tablet_id,
+                                         const scoped_refptr<RemoteTablet>& rt,
                                          ReplicaSelection selection,
                                          const set<string>& blacklist,
                                          vector<RemoteTabletServer*>* candidates,
                                          RemoteTabletServer** ts) {
   // TODO: write a proper async version of this for async client.
-  scoped_refptr<RemoteTablet> remote_tablet;
-  meta_cache_->LookupTabletByID(tablet_id, &remote_tablet);
-
-  RemoteTabletServer* ret = SelectTServer(remote_tablet, selection, blacklist, candidates);
+  RemoteTabletServer* ret = SelectTServer(rt, selection, blacklist, candidates);
   if (PREDICT_FALSE(ret == nullptr)) {
     // Construct a blacklist string if applicable.
     string blacklist_string = "";
@@ -314,11 +311,11 @@ Status KuduClient::Data::GetTabletServer(KuduClient* client,
     return Status::ServiceUnavailable(
         Substitute("No $0 for tablet $1 $2",
                    selection == LEADER_ONLY ? "LEADER" : "replicas",
-                   tablet_id,
+                   rt->tablet_id(),
                    blacklist_string));
   }
   Synchronizer s;
-  ret->RefreshProxy(client, s.AsStatusCallback(), false);
+  ret->InitProxy(client, s.AsStatusCallback());
   RETURN_NOT_OK(s.Wait());
 
   *ts = ret;

http://git-wip-us.apache.org/repos/asf/incubator-kudu/blob/a10e3739/src/kudu/client/client-internal.h
----------------------------------------------------------------------
diff --git a/src/kudu/client/client-internal.h b/src/kudu/client/client-internal.h
index 81a2f43..fa04b9c 100644
--- a/src/kudu/client/client-internal.h
+++ b/src/kudu/client/client-internal.h
@@ -53,7 +53,7 @@ class KuduClient::Data {
   Data();
   ~Data();
 
-  // Returns a ts that hosts a tablet with the given tablet ID, subject
+  // Selects a TS replica from the given RemoteTablet subject
   // to liveness and the provided selection criteria and blacklist.
   //
   // If no appropriate replica can be found, a non-OK status is returned and 'ts' is untouched.
@@ -62,7 +62,7 @@ class KuduClient::Data {
   // criteria, but are possibly filtered by the blacklist. This is useful for implementing
   // retry logic.
   Status GetTabletServer(KuduClient* client,
-                         const std::string& tablet_id,
+                         const scoped_refptr<internal::RemoteTablet>& rt,
                          ReplicaSelection selection,
                          const std::set<std::string>& blacklist,
                          std::vector<internal::RemoteTabletServer*>* candidates,

http://git-wip-us.apache.org/repos/asf/incubator-kudu/blob/a10e3739/src/kudu/client/client-test.cc
----------------------------------------------------------------------
diff --git a/src/kudu/client/client-test.cc b/src/kudu/client/client-test.cc
index 332b8b3..21a398f 100644
--- a/src/kudu/client/client-test.cc
+++ b/src/kudu/client/client-test.cc
@@ -1042,25 +1042,25 @@ TEST_F(ClientTest, TestGetTabletServerBlacklist) {
   set<string> blacklist;
   vector<internal::RemoteTabletServer*> candidates;
   vector<internal::RemoteTabletServer*> tservers;
-  ASSERT_OK(client_->data_->GetTabletServer(client_.get(), rt->tablet_id(),
+  ASSERT_OK(client_->data_->GetTabletServer(client_.get(), rt,
                                             KuduClient::LEADER_ONLY,
                                             blacklist, &candidates, &rts));
   tservers.push_back(rts);
   // Blacklist the leader, should not work.
   blacklist.insert(rts->permanent_uuid());
   {
-    Status s = client_->data_->GetTabletServer(client_.get(), rt->tablet_id(),
+    Status s = client_->data_->GetTabletServer(client_.get(), rt,
                                                KuduClient::LEADER_ONLY,
                                                blacklist, &candidates, &rts);
     ASSERT_TRUE(s.IsServiceUnavailable());
   }
   // Keep blacklisting replicas until we run out.
-  ASSERT_OK(client_->data_->GetTabletServer(client_.get(), rt->tablet_id(),
+  ASSERT_OK(client_->data_->GetTabletServer(client_.get(), rt,
                                             KuduClient::CLOSEST_REPLICA,
                                             blacklist, &candidates, &rts));
   tservers.push_back(rts);
   blacklist.insert(rts->permanent_uuid());
-  ASSERT_OK(client_->data_->GetTabletServer(client_.get(), rt->tablet_id(),
+  ASSERT_OK(client_->data_->GetTabletServer(client_.get(), rt,
                                             KuduClient::FIRST_REPLICA,
                                             blacklist, &candidates, &rts));
   tservers.push_back(rts);
@@ -1072,7 +1072,7 @@ TEST_F(ClientTest, TestGetTabletServerBlacklist) {
   selections.push_back(KuduClient::CLOSEST_REPLICA);
   selections.push_back(KuduClient::FIRST_REPLICA);
   for (KuduClient::ReplicaSelection selection : selections) {
-    Status s = client_->data_->GetTabletServer(client_.get(), rt->tablet_id(), selection,
+    Status s = client_->data_->GetTabletServer(client_.get(), rt, selection,
                                                blacklist, &candidates, &rts);
     ASSERT_TRUE(s.IsServiceUnavailable());
   }
@@ -1083,7 +1083,7 @@ TEST_F(ClientTest, TestGetTabletServerBlacklist) {
   }
   blacklist.clear();
   for (KuduClient::ReplicaSelection selection : selections) {
-    Status s = client_->data_->GetTabletServer(client_.get(), rt->tablet_id(),
+    Status s = client_->data_->GetTabletServer(client_.get(), rt,
                                                selection,
                                                blacklist, &candidates, &rts);
     ASSERT_TRUE(s.IsServiceUnavailable());
@@ -2155,7 +2155,7 @@ TEST_F(ClientTest, TestReplicatedTabletWritesWithLeaderElection) {
   set<string> blacklist;
   vector<internal::RemoteTabletServer*> candidates;
   ASSERT_OK(client_->data_->GetTabletServer(client_.get(),
-                                            rt->tablet_id(),
+                                            rt,
                                             KuduClient::LEADER_ONLY,
                                             blacklist,
                                             &candidates,

http://git-wip-us.apache.org/repos/asf/incubator-kudu/blob/a10e3739/src/kudu/client/meta_cache.cc
----------------------------------------------------------------------
diff --git a/src/kudu/client/meta_cache.cc b/src/kudu/client/meta_cache.cc
index fbc4856..8d7e427 100644
--- a/src/kudu/client/meta_cache.cc
+++ b/src/kudu/client/meta_cache.cc
@@ -94,14 +94,12 @@ void RemoteTabletServer::DnsResolutionFinished(const HostPort& hp,
   user_callback.Run(s);
 }
 
-void RemoteTabletServer::RefreshProxy(KuduClient* client,
-                                      const StatusCallback& cb,
-                                      bool force) {
+void RemoteTabletServer::InitProxy(KuduClient* client, const StatusCallback& cb) {
   HostPort hp;
   {
     unique_lock<simple_spinlock> l(&lock_);
 
-    if (proxy_ && !force) {
+    if (proxy_) {
       // Already have a proxy created.
       l.unlock();
       cb.Run(Status::OK());
@@ -611,12 +609,6 @@ void MetaCache::LookupTabletByKey(const KuduTable* table,
   rpc->SendRpc();
 }
 
-void MetaCache::LookupTabletByID(const string& tablet_id,
-                                 scoped_refptr<RemoteTablet>* remote_tablet) {
-  shared_lock<rw_spinlock> l(&lock_);
-  *remote_tablet = FindOrDie(tablets_by_id_, tablet_id);
-}
-
 void MetaCache::MarkTSFailed(RemoteTabletServer* ts,
                              const Status& status) {
   LOG(INFO) << "Marking tablet server " << ts->ToString() << " as failed.";

http://git-wip-us.apache.org/repos/asf/incubator-kudu/blob/a10e3739/src/kudu/client/meta_cache.h
----------------------------------------------------------------------
diff --git a/src/kudu/client/meta_cache.h b/src/kudu/client/meta_cache.h
index 44a11d5..22caf84 100644
--- a/src/kudu/client/meta_cache.h
+++ b/src/kudu/client/meta_cache.h
@@ -69,16 +69,16 @@ class RemoteTabletServer {
  public:
   explicit RemoteTabletServer(const master::TSInfoPB& pb);
 
-  // Refresh the RPC proxy to this tablet server. This may involve a DNS
-  // lookup if there is not already an active proxy.
-  void RefreshProxy(KuduClient* client, const StatusCallback& cb,
-                    bool force);
+  // Initialize the RPC proxy to this tablet server, if it is not already set up.
+  // This will involve a DNS lookup if there is not already an active proxy.
+  // If there is an active proxy, does nothing.
+  void InitProxy(KuduClient* client, const StatusCallback& cb);
 
   // Update information from the given pb.
   // Requires that 'pb''s UUID matches this server.
   void Update(const master::TSInfoPB& pb);
 
-  // Return the current proxy to this tablet server. Requires that RefreshProxy
+  // Return the current proxy to this tablet server. Requires that InitProxy()
   // be called prior to this.
   std::shared_ptr<tserver::TabletServerServiceProxy> proxy() const;
 
@@ -216,13 +216,6 @@ class MetaCache : public RefCountedThreadSafe<MetaCache> {
                          scoped_refptr<RemoteTablet>* remote_tablet,
                          const StatusCallback& callback);
 
-  // Look up the RemoteTablet object for the given tablet ID. Will die if not
-  // found.
-  //
-  // This is always a local operation (no network round trips or DNS resolution, etc).
-  void LookupTabletByID(const std::string& tablet_id,
-                        scoped_refptr<RemoteTablet>* remote_tablet);
-
   // Mark any replicas of any tablets hosted by 'ts' as failed. They will
   // not be returned in future cache lookups.
   void MarkTSFailed(RemoteTabletServer* ts, const Status& status);

http://git-wip-us.apache.org/repos/asf/incubator-kudu/blob/a10e3739/src/kudu/client/scanner-internal.cc
----------------------------------------------------------------------
diff --git a/src/kudu/client/scanner-internal.cc b/src/kudu/client/scanner-internal.cc
index 187c943..852a4c3 100644
--- a/src/kudu/client/scanner-internal.cc
+++ b/src/kudu/client/scanner-internal.cc
@@ -268,7 +268,7 @@ Status KuduScanner::Data::OpenTablet(const string& partition_key,
     vector<RemoteTabletServer*> candidates;
     Status lookup_status = table_->client()->data_->GetTabletServer(
         table_->client(),
-        remote_->tablet_id(),
+        remote_,
         selection_,
         *blacklist,
         &candidates,


Mime
View raw message