[GitHub] [accumulo] ctubbsii commented on a diff in pull request #3103: Rework #3047 so that the client address is captured correctly

[GitBox <gi...@apache.org>]

ctubbsii commented on code in PR #3103:
URL: https://github.com/apache/accumulo/pull/3103#discussion_r1038411530


##########
server/base/src/main/java/org/apache/accumulo/server/rpc/CustomNonBlockingServer.java:
##########
@@ -112,29 +112,20 @@ protected FrameBuffer createFrameBuffer(final TNonblockingTransport trans,
    * extract the client's network location before accepting the request.
    */
   private class CustomFrameBuffer extends FrameBuffer {
+    private final String clientAddress;
 
     public CustomFrameBuffer(TNonblockingTransport trans, SelectionKey selectionKey,
         AbstractSelectThread selectThread) throws TTransportException {
       super(trans, selectionKey, selectThread);
-    }
-
-    @Override
-    public void invoke() {
-      if (trans_ instanceof TNonblockingSocket) {
-        TNonblockingSocket tsock = (TNonblockingSocket) trans_;
-        Socket sock = tsock.getSocketChannel().socket();
-        TServerUtils.clientAddress
-            .set(sock.getInetAddress().getHostAddress() + ":" + sock.getPort());
-      }
-      super.invoke();
+      this.clientAddress = getClientAddress();

Review Comment:
   This new method helps log it locally... but the TServerUtils.clientAddress ThreadLocal is used elsewhere as well. Since this change causes it to no longer be set at all, it's possibly causing it to be always null for those other places, even though it fixes it here.
   
   Also, it was previously done in the invoke method instead of the constructor. I'm not exactly sure why that was the case, but I suspect it's because the CustomFrameBuffer can be reused for different connections, and the invoke method is called again each time it is reused. If that is the case, and this is reused, then the client address will never be updated if it's set in the constructor, and we'll log for the wrong client.



##########
server/base/src/main/java/org/apache/accumulo/server/rpc/CustomNonBlockingServer.java:
##########
@@ -144,11 +135,30 @@ public boolean write() {
       boolean result = super.write();
       if (!result) {
         log.trace("CustomFrameBuffer.write returned false when writing data to client: {}",
-            TServerUtils.clientAddress.get());
+            clientAddress);
       }
       return result;
     }
 
+    /*
+     * Helper method used to capture the client address from the socket for each new allocated
+     * FrameBuffer. Because this is a non-blocking server there is a shared thread pool so threads
+     * are re-used for different requests. A new FrameBuffer is created for each request and
+     * different threads can process the same FrameBuffer (For example read() and invoke() can be
+     * called on different threads) so trying to store the client address in a thread local does not
+     * work. The simplest thing to do is to get the client address on FrameBuffer creation as the
+     * same FrameBuffer will be used for the duration of the request.
+     */
+    private String getClientAddress() {
+      String clientAddress = null;
+      if (trans_ instanceof TNonblockingSocket) {

Review Comment:
   I thought other server types had similar code. When I looked for where the ThreadLocal was being set, I think I saw two places... one for this server type, and one for another server type (maybe the server type we use with SSL or Kerberos?)



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@accumulo.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org