Removing CustomThreadedSelectorServer.java

[Amemeda]

Investigated relevance of CustomThreadedSelectorServer.java

• This file overrides getRunnable method from TThreadedSelectorServer.java.
• CustomThreadedSelectorServer.java had only 1 usage in TServerUtils.java: return new ServerAddress(new CustomThreadedSelectorServer(options), address);

Result:

• Deleted file CustomThreadedSelectorServer.java
• replaced its 1 usage with TThreadedSelectorServer.java

[Amemeda]

@ctubbsii

[ctubbsii]

I believe all of this custom stuff is just to extract this client address from the socket, so we can use it elsewhere (probably for logging). I don't think we can delete this without replacing it with a different mechanism to get the client address.

The field we are using reflection to read used to be accessible in previous versions of Thrift, and then it was made private, which caused us to make this class to work around that. We may still need this workaround, unless Thrift provided another means to obtain the client address from the connection.

[dlmarion]

It looks like TSocket, TNonblockingSocket and TNonblockingSSLSocket now implement https://github.com/apache/thrift/blob/master/lib/java/src/main/java/org/apache/thrift/transport/SocketAddressProvider.java. We should probably use those methods to get both the local and remote addresses.

[Amemeda]

So then TThreadedSelectorServer.java should also implement SocketAddressProvider and override those methods? I was going to try that out but TThreadedSelectorServer is read only for me

[dlmarion]

Looking at the Thrift code, I think the only change here based on my comment above is:

-          Socket sock = tsock.getSocketChannel().socket();
-          TServerUtils.clientAddress
-              .set(sock.getInetAddress().getHostAddress() + ":" + sock.getPort());
+          SocketAddress addr = tsock.getRemoteSocketAddress();
+          if (addr != null) {
+            TServerUtils.clientAddress.set(addr.toString());
+          }

I think we still need the CustomThreadedSelectorServer class that uses reflection to get access to the transport.

[dlmarion]

Actually, I think I found something else, I'll write it up

[dlmarion]

org.apache.thrift.server.TServer has a method called setServerEventHandler. We would need to create our own instance of TServerEventHandler and ServerContext, and then call the setServerEventHandler method on each TServer that we create.

When the event handler is set, then FrameBuffer, TSimpleServer, and some other code call TServerEventHandler.createContext followed by ServerContext.setRemoteAddress. I'm not sure how we would wire this up yet to the TServerUtils.clientAddress thread local, but I think this might give us what we need to get rid of the custom class.

We would want to call TServerUtils.clientAddress.set() when ServerContext.setRemoteAddress is called, and then clear it when TServerEventHandler.deleteContext is called.