Merge pull request #38 from twitter/connection-reuse-fixes

Connection reuse fixes

lib/gizzard/nameserver.rb

@@ -145,33 +145,33 @@ def initialize(*hosts)
     end
 
     def get_shards(ids)
-      ids.map {|id| with_retry("get_shards", 1) { random_client.get_shard(id) } }
+      ids.map {|id| with_retry(1) { random_client.get_shard(id) } }
     end
 
     def reload_updated_forwardings
       opname = "reload_updated_forwardings"
       on_all_servers opname do |c|
-        with_retry(opname, MAX_BACKOFF_SECS/4) { c.reload_updated_forwardings }
+        with_retry(MAX_BACKOFF_SECS/4) { c.reload_updated_forwardings }
       end
     end
 
     def reload_config
       opname = "reload_config"
       on_all_servers opname do |c|
-        with_retry(opname, MAX_BACKOFF_SECS/4) { c.reload_config }
+        with_retry(MAX_BACKOFF_SECS/4) { c.reload_config }
       end
     end
 
     def copy_shard(*shards)
-      with_retry("copy_shard", MAX_BACKOFF_SECS/2) { random_client.copy_shard(*shards) }
+      with_retry(MAX_BACKOFF_SECS/2) { random_client.copy_shard(*shards) }
     end
 
     def repair_shards(*shards)
-      with_retry("repair_shards", MAX_BACKOFF_SECS/2) { random_client.repair_shard(*shards) }
+      with_retry(MAX_BACKOFF_SECS/2) { random_client.repair_shard(*shards) }
     end
 
     def diff_shards(*shards)
-      with_retry("diff_shards", MAX_BACKOFF_SECS/2) { random_client.diff_shards(*shards) }
+      with_retry(MAX_BACKOFF_SECS/2) { random_client.diff_shards(*shards) }
     end
 
     def respond_to?(method)
@@ -182,7 +182,7 @@ def method_missing(method, *args, &block)
       if client.respond_to?(method)
         # operations without specialized backoff use a backoff which assumes cheap, easily
         # retryable operations: if this isn't the case, methods should specialize as above
-        with_retry(method, 0.1) { random_client.send(method, *args, &block) }
+        with_retry(0.1) { random_client.send(method, *args, &block) }
       else
         super
       end
@@ -299,14 +299,13 @@ def create_client(host)
 
     private
 
-    def with_retry(opname, min_backoff_secs)
+    def with_retry(min_backoff_secs)
       times ||= @retries
       yield
     rescue SystemExit => e
-      STDERR.puts "\nExiting immediately for #{e.to_s}"
+      STDERR.puts "Exiting immediately for #{e.to_s}"
       raise
     rescue Exception => e
-      STDERR.puts "\nException for #{opname}: #{e.to_s}: #{e.description rescue "(no description)"}"
       STDERR.puts "Retrying #{times} more time#{'s' if times > 1}..." if times > 0
       times -= 1
       sleep_time = [min_backoff_secs, MAX_BACKOFF_SECS / [times, 1].max].max

lib/gizzard/thrift.rb

@@ -299,12 +299,12 @@ class Manager < GizzmoService
     # (log entries are binary, but this client doesn't support labeling them as such)
 
     # create or get a log, returning a binary id for that log
-    thrift_method :log_create, string, field(:log_name, string, 1)
-    thrift_method :log_get, string, field(:log_name, string, 1)
+    thrift_method :log_create, string, field(:log_name, string, 1), :throws => exception(GizzardException)
+    thrift_method :log_get, string, field(:log_name, string, 1), :throws => exception(GizzardException)
     # given a log id, push/pop/peek binary data on that log
-    thrift_method :log_entry_push, void, field(:log_id, string, 1), field(:log_entry, struct(LogEntry), 2)
-    thrift_method :log_entry_peek, list(struct(LogEntry)), field(:log_id, string, 1), field(:count, i32, 2)
-    thrift_method :log_entry_pop, void, field(:log_id, string, 1), field(:log_entry_id, i32, 2)
+    thrift_method :log_entry_push, void, field(:log_id, string, 1), field(:log_entry, struct(LogEntry), 2), :throws => exception(GizzardException)
+    thrift_method :log_entry_peek, list(struct(LogEntry)), field(:log_id, string, 1), field(:count, i32, 2), :throws => exception(GizzardException)
+    thrift_method :log_entry_pop, void, field(:log_id, string, 1), field(:log_entry_id, i32, 2), :throws => exception(GizzardException)
   end
 
 

lib/vendor/thrift_client/simple.rb

@@ -34,7 +34,7 @@ module Simple
     # failed rpc
     SOCKETS = Hash.new
     previous_sig = trap("EXIT") do
-      SOCKETS.each do |sock|
+      SOCKETS.each do |host,sock|
         begin
           sock.close
         rescue
@@ -44,12 +44,10 @@ module Simple
 
     # establish or reuse a persistent connection for the given host
     def self.sock(host, port)
-      if SOCKETS.key? host
-        SOCKETS[host]
-      else
-        sock = TCPSocket.new(host, port)
-        SOCKETS[host] = sock
+      if !SOCKETS.key?(host)
+        SOCKETS[host] = TCPSocket.new(host, port)
       end
+      SOCKETS[host]
     end
 
     # attempt to close and clear the connection for the given host
@@ -355,11 +353,11 @@ def _proxy(method_name, *args)
         arg_struct = arg_class.new(*args)
         begin
           sock = ThriftClient::Simple.sock(@host, @port)
-          packed = ThriftClient::Simple.pack_request(method_name, arg_struct, @framed)
-          wrote = sock.write(packed)
+          sock.write(ThriftClient::Simple.pack_request(method_name, arg_struct, @framed))
           rv = ThriftClient::Simple.read_response(sock, rv_class, @framed)
           rv[2]
         rescue Exception => e
+          STDERR.puts "\nException for #{method_name} on #{@host}: #{e.to_s}: #{e.description rescue "(no description)"}"
           ThriftClient::Simple.sock_close(@host)
           raise e
         end