You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "Amemeda (via GitHub)" <gi...@apache.org> on 2026/06/30 15:52:06 UTC
[PR] Removing CustomThreadedSelectorServer.java [accumulo]
Amemeda opened a new pull request, #6454:
URL: https://github.com/apache/accumulo/pull/6454
(no comment)
--
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
Re: [PR] Removing CustomThreadedSelectorServer.java [accumulo]
Posted by "Amemeda (via GitHub)" <gi...@apache.org>.
Amemeda commented on code in PR #6454:
URL: https://github.com/apache/accumulo/pull/6454#discussion_r3530063952
##########
server/base/src/main/java/org/apache/accumulo/server/rpc/CustomThreadedSelectorServer.java:
##########
@@ -1,74 +0,0 @@
-/*
- * Licensed to the Apache Software Foundation (ASF) under one
- * or more contributor license agreements. See the NOTICE file
- * distributed with this work for additional information
- * regarding copyright ownership. The ASF licenses this file
- * to you under the Apache License, Version 2.0 (the
- * "License"); you may not use this file except in compliance
- * with the License. You may obtain a copy of the License at
- *
- * https://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing,
- * software distributed under the License is distributed on an
- * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
- * KIND, either express or implied. See the License for the
- * specific language governing permissions and limitations
- * under the License.
- */
-package org.apache.accumulo.server.rpc;
-
-import java.lang.reflect.Field;
-import java.net.Socket;
-
-import org.apache.thrift.server.TThreadedSelectorServer;
-import org.apache.thrift.transport.TNonblockingSocket;
-import org.apache.thrift.transport.TNonblockingTransport;
-import org.slf4j.LoggerFactory;
-
-public class CustomThreadedSelectorServer extends TThreadedSelectorServer {
-
- private final Field fbTansportField;
-
- public CustomThreadedSelectorServer(Args args) {
- super(args);
-
- try {
- fbTansportField = FrameBuffer.class.getDeclaredField("trans_");
- fbTansportField.setAccessible(true);
- } catch (SecurityException | NoSuchFieldException e) {
- throw new IllegalStateException("Failed to access required field in Thrift code.", e);
- }
- }
-
- private TNonblockingTransport getTransport(FrameBuffer frameBuffer) {
- try {
- return (TNonblockingTransport) fbTansportField.get(frameBuffer);
- } catch (IllegalAccessException e) {
- throw new IllegalStateException(e);
- }
- }
-
- @Override
- protected Runnable getRunnable(FrameBuffer frameBuffer) {
- return () -> {
-
- try {
- TNonblockingTransport transport = getTransport(frameBuffer);
-
- if (transport instanceof TNonblockingSocket tsock) {
- // This block of code makes the client address available to the server side code that
- // executes a RPC. It is made available for informational purposes.
- Socket sock = tsock.getSocketChannel().socket();
- TServerUtils.clientAddress
- .set(sock.getInetAddress().getHostAddress() + ":" + sock.getPort());
Review Comment:
This change needs to be in CustomThreadedSelectorServer.java?
--
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
Re: [PR] Removing CustomThreadedSelectorServer.java [accumulo]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #6454:
URL: https://github.com/apache/accumulo/pull/6454#discussion_r3502330015
##########
server/base/src/main/java/org/apache/accumulo/server/rpc/CustomThreadedSelectorServer.java:
##########
@@ -1,74 +0,0 @@
-/*
- * Licensed to the Apache Software Foundation (ASF) under one
- * or more contributor license agreements. See the NOTICE file
- * distributed with this work for additional information
- * regarding copyright ownership. The ASF licenses this file
- * to you under the Apache License, Version 2.0 (the
- * "License"); you may not use this file except in compliance
- * with the License. You may obtain a copy of the License at
- *
- * https://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing,
- * software distributed under the License is distributed on an
- * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
- * KIND, either express or implied. See the License for the
- * specific language governing permissions and limitations
- * under the License.
- */
-package org.apache.accumulo.server.rpc;
-
-import java.lang.reflect.Field;
-import java.net.Socket;
-
-import org.apache.thrift.server.TThreadedSelectorServer;
-import org.apache.thrift.transport.TNonblockingSocket;
-import org.apache.thrift.transport.TNonblockingTransport;
-import org.slf4j.LoggerFactory;
-
-public class CustomThreadedSelectorServer extends TThreadedSelectorServer {
-
- private final Field fbTansportField;
-
- public CustomThreadedSelectorServer(Args args) {
- super(args);
-
- try {
- fbTansportField = FrameBuffer.class.getDeclaredField("trans_");
- fbTansportField.setAccessible(true);
- } catch (SecurityException | NoSuchFieldException e) {
- throw new IllegalStateException("Failed to access required field in Thrift code.", e);
- }
- }
-
- private TNonblockingTransport getTransport(FrameBuffer frameBuffer) {
- try {
- return (TNonblockingTransport) fbTansportField.get(frameBuffer);
- } catch (IllegalAccessException e) {
- throw new IllegalStateException(e);
- }
- }
-
- @Override
- protected Runnable getRunnable(FrameBuffer frameBuffer) {
- return () -> {
-
- try {
- TNonblockingTransport transport = getTransport(frameBuffer);
-
- if (transport instanceof TNonblockingSocket tsock) {
- // This block of code makes the client address available to the server side code that
- // executes a RPC. It is made available for informational purposes.
- Socket sock = tsock.getSocketChannel().socket();
- TServerUtils.clientAddress
- .set(sock.getInetAddress().getHostAddress() + ":" + sock.getPort());
Review Comment:
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.
--
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
Re: [PR] Remove Custom...Server.java classes and replace with Thrift implementation [accumulo]
Posted by "DomGarguilo (via GitHub)" <gi...@apache.org>.
DomGarguilo commented on PR #6454:
URL: https://github.com/apache/accumulo/pull/6454#issuecomment-5318502560
I reran the failing job and its passing now. Not sure what caused the failures though
--
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
Re: [PR] Remove Custom...Server.java classes and replace with Thrift implementation [accumulo]
Posted by "Amemeda (via GitHub)" <gi...@apache.org>.
Amemeda commented on PR #6454:
URL: https://github.com/apache/accumulo/pull/6454#issuecomment-5171911216
> 4. I didn't actually run it to verify that the trace log messages or the audit logs were recording the client address. Please check that and verify that it is still logging correctly.
>
Here is what I was able to grab for verifying the client address,
<img width="994" height="166" alt="image" src="https://github.com/user-attachments/assets/d0765508-59b1-4651-800a-6d175a9a1259" />
<img width="1000" height="195" alt="image" src="https://github.com/user-attachments/assets/49872bee-9815-4cc4-b0c8-87ec487ec6b2" />
--
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
Re: [PR] Remove Custom...Server.java classes and replace with Thrift implementation [accumulo]
Posted by "Amemeda (via GitHub)" <gi...@apache.org>.
Amemeda commented on code in PR #6454:
URL: https://github.com/apache/accumulo/pull/6454#discussion_r3766945903
##########
server/base/src/main/java/org/apache/accumulo/server/rpc/ThriftServerType.java:
##########
@@ -43,10 +45,13 @@ private ThriftServerType(String name) {
}
public static ThriftServerType get(String name) {
+ final Logger log = LoggerFactory.getLogger(ThriftServerType.class);
if (StringUtils.isBlank(name)) {
return getDefault();
}
+ // check added for backwards compatibility
if (name.trim().equalsIgnoreCase("custom_hs_ha")) {
+ log.warn("Server type custom_hs_ha is deprecated, use {}", ThriftServerType.HS_HA);
Review Comment:
Sorry about that I didnt see Dave's response, Yes I kept the warn as suggested by Christopher, and also because I confirmed warn is used elsewhere in the code for deprecated items
--
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
Re: [PR] Remove Custom...Server.java classes and replace with Thrift implementation [accumulo]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #6454:
URL: https://github.com/apache/accumulo/pull/6454#discussion_r3648297655
##########
server/base/src/main/java/org/apache/accumulo/server/rpc/TServerUtils.java:
##########
@@ -225,7 +228,10 @@ private static ServerAddress createNonBlockingServer(HostAndPort address, TProce
address = HostAndPort.fromParts(address.getHost(), transport.getPort());
}
- return new ServerAddress(new CustomNonBlockingServer(options), address);
+ final THsHaServer server = new THsHaServer(options);
Review Comment:
In e2efd0400d4206de188d4960b273892d725f3a82, we added an override to the stop method because the upstream implementation does not attempt to shut down its internal executor service when stop is requested. In fact, it previously existed using reflection, but this commit cleaned up the implementation.
I think we probably still need that here, although, we don't need to override the whole class. We can probably just do it inline here, something like:
```suggestion
final THsHaServer server = new THsHaServer(options) {
@Override
public void stop() {
super.stop();
try {
getInvoker().shutdownNow();
} catch (Exception e) {
log.error("Unable to call shutdownNow", e);
}
}
};
```
This is probably a bugfix we can contribute to the Thrift project, so we don't need to maintain this indefinitely, but I think we still want it for now.
--
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
Re: [PR] Removing CustomThreadedSelectorServer.java [accumulo]
Posted by "Amemeda (via GitHub)" <gi...@apache.org>.
Amemeda commented on PR #6454:
URL: https://github.com/apache/accumulo/pull/6454#issuecomment-4845580328
@ctubbsii
--
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
Re: [PR] Remove Custom...Server.java classes and replace with Thrift implementation [accumulo]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #6454:
URL: https://github.com/apache/accumulo/pull/6454#discussion_r3694402623
##########
server/base/src/main/java/org/apache/accumulo/server/rpc/TServerUtils.java:
##########
@@ -295,6 +312,8 @@ private static ServerAddress createBlockingServer(HostAndPort address, TProcesso
log.info("Blocking Server bound on {}", address);
}
+ server.setServerEventHandler(new ThriftServerEventHandler());
+
return new ServerAddress(server, address);
Review Comment:
This set event handler, followed by a return of the server address seems to be boilerplate stuff that could be done in one place. But, it's also pretty trivial, so may not be worth refactoring.
##########
server/base/src/main/java/org/apache/accumulo/server/rpc/TServerUtils.java:
##########
Review Comment:
Interesting. I didn't realize we were passing in our own executor service here. Since that's the case, we have more options than overriding the stop method. We could also set up a Cleaner to stop the thread pool when the server gets garbage collected, or shut it down some other way.
##########
server/base/src/main/java/org/apache/accumulo/server/rpc/TServerUtils.java:
##########
@@ -225,7 +228,21 @@ private static ServerAddress createNonBlockingServer(HostAndPort address, TProce
address = HostAndPort.fromParts(address.getHost(), transport.getPort());
}
- return new ServerAddress(new CustomNonBlockingServer(options), address);
+ final THsHaServer server = new THsHaServer(options) {
+ @Override
+ public void stop() {
+ super.stop();
+ try {
+ getInvoker().shutdownNow();
+ } catch (Exception e) {
+ log.error("Unable to call shutdownNow", e);
+ }
+ }
+ };
Review Comment:
This is fine, since it preserves our existing behavior. We can consider other options at a future time, if we need to, but this should work fine.
##########
server/base/src/main/java/org/apache/accumulo/server/rpc/TServerUtils.java:
##########
@@ -395,9 +417,11 @@ private static ServerAddress createSslThreadPoolServer(HostAndPort address, TPro
ThreadPoolExecutor pool =
createSelfResizingThreadPool(numThreads, threadTimeOut, conf, timeBetweenThreadChecks);
+ TThreadPoolServer server = createTThreadPoolServer(transport, processor,
+ ThriftUtil.transportFactory(), protocolFactory, pool);
+ server.setServerEventHandler(new ThriftServerEventHandler());
Review Comment:
It looks like some of these might be redundant. This calls createTThreadPoolServer(), which already sets the event handler. It's not a problem, but it's an unnecessary object that's created. This would be an argument for refactoring to consolidate that boilerplate, but the object is very lightweight and is called once at startup, so it's probably not causing any harm being redundant.
##########
server/base/src/main/java/org/apache/accumulo/server/rpc/ThriftServerType.java:
##########
Review Comment:
This method will return null if somebody had configured the old value instead of relying on the default. So, you can add a line here, like:
```java
if (name.trim().equalsIgnoreCase("custom_hs_ha")) {
return ThriftServerType.HS_HA;
}
```
--
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
Re: [PR] Remove Custom...Server.java classes and replace with Thrift implementation [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #6454:
URL: https://github.com/apache/accumulo/pull/6454#discussion_r3737280907
##########
server/base/src/main/java/org/apache/accumulo/server/rpc/ThriftServerType.java:
##########
@@ -43,10 +45,13 @@ private ThriftServerType(String name) {
}
public static ThriftServerType get(String name) {
+ final Logger log = LoggerFactory.getLogger(ThriftServerType.class);
if (StringUtils.isBlank(name)) {
return getDefault();
}
+ // check added for backwards compatibility
if (name.trim().equalsIgnoreCase("custom_hs_ha")) {
+ log.warn("Server type custom_hs_ha is deprecated, use {}", ThriftServerType.HS_HA);
Review Comment:
Suggest looking at other deprecation type logging, probably around Property.java, and see if `warn` is appropriate
##########
server/base/src/main/java/org/apache/accumulo/server/rpc/ThriftServerType.java:
##########
@@ -43,10 +45,13 @@ private ThriftServerType(String name) {
}
public static ThriftServerType get(String name) {
+ final Logger log = LoggerFactory.getLogger(ThriftServerType.class);
Review Comment:
This should be moved to the top of the class using the `private static final ` modifiers.
--
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
Re: [PR] Remove Custom...Server.java classes and replace with Thrift implementation [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on PR #6454:
URL: https://github.com/apache/accumulo/pull/6454#issuecomment-5105575762
I think `ThriftServerType.CUSTOM_HS_HA` needs to be renamed to remove `CUSTOM_` from the enum name and value.
--
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
Re: [PR] Removing Custom...Server.java classes and replacing with Thrift implementation [accumulo]
Posted by "Amemeda (via GitHub)" <gi...@apache.org>.
Amemeda commented on code in PR #6454:
URL: https://github.com/apache/accumulo/pull/6454#discussion_r3554342774
##########
server/base/src/main/java/org/apache/accumulo/server/rpc/TServerUtils.java:
##########
@@ -225,7 +228,10 @@ private static ServerAddress createNonBlockingServer(HostAndPort address, TProce
address = HostAndPort.fromParts(address.getHost(), transport.getPort());
}
- return new ServerAddress(new CustomNonBlockingServer(options), address);
+ CustomNonBlockingServer server = new CustomNonBlockingServer(options);
Review Comment:
Ive removed that one, I think that's all the custom server classes. The only remaining custom classes I see are CustomObjectMapper and CustomHsHaNestedIT
--
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
Re: [PR] Remove Custom...Server.java classes and replace with Thrift implementation [accumulo]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #6454:
URL: https://github.com/apache/accumulo/pull/6454#discussion_r3694259307
##########
server/base/src/main/java/org/apache/accumulo/server/rpc/TServerUtils.java:
##########
@@ -225,7 +228,10 @@ private static ServerAddress createNonBlockingServer(HostAndPort address, TProce
address = HostAndPort.fromParts(address.getHost(), transport.getPort());
}
- return new ServerAddress(new CustomNonBlockingServer(options), address);
+ final THsHaServer server = new THsHaServer(options);
Review Comment:
I started looking into submitting a patch upstream to Thrift, and some of the code they have there seems worryingly bad. They have a graceful shutdown method for the invoker, but they never call it. It also uses awaitTermination but doesn't check the return boolean, and the loop uses the system clock rather (currentTimeMillis) than the CPU clock (nanoTime).
Upstream also allows the executor to be passed in as an argument, so you can provide a different implementation. But also that allows you to manage its lifecycle outside of this server's lifecycle, but I'm not sure that's a great idea.
In short, I think it's fine to keep our patch for now. Submitting a patch upstream isn't straight-forward... as it's not clear what the right design should be.
--
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
Re: [PR] Removing CustomThreadedSelectorServer.java [accumulo]
Posted by "Amemeda (via GitHub)" <gi...@apache.org>.
Amemeda commented on code in PR #6454:
URL: https://github.com/apache/accumulo/pull/6454#discussion_r3530063952
##########
server/base/src/main/java/org/apache/accumulo/server/rpc/CustomThreadedSelectorServer.java:
##########
@@ -1,74 +0,0 @@
-/*
- * Licensed to the Apache Software Foundation (ASF) under one
- * or more contributor license agreements. See the NOTICE file
- * distributed with this work for additional information
- * regarding copyright ownership. The ASF licenses this file
- * to you under the Apache License, Version 2.0 (the
- * "License"); you may not use this file except in compliance
- * with the License. You may obtain a copy of the License at
- *
- * https://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing,
- * software distributed under the License is distributed on an
- * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
- * KIND, either express or implied. See the License for the
- * specific language governing permissions and limitations
- * under the License.
- */
-package org.apache.accumulo.server.rpc;
-
-import java.lang.reflect.Field;
-import java.net.Socket;
-
-import org.apache.thrift.server.TThreadedSelectorServer;
-import org.apache.thrift.transport.TNonblockingSocket;
-import org.apache.thrift.transport.TNonblockingTransport;
-import org.slf4j.LoggerFactory;
-
-public class CustomThreadedSelectorServer extends TThreadedSelectorServer {
-
- private final Field fbTansportField;
-
- public CustomThreadedSelectorServer(Args args) {
- super(args);
-
- try {
- fbTansportField = FrameBuffer.class.getDeclaredField("trans_");
- fbTansportField.setAccessible(true);
- } catch (SecurityException | NoSuchFieldException e) {
- throw new IllegalStateException("Failed to access required field in Thrift code.", e);
- }
- }
-
- private TNonblockingTransport getTransport(FrameBuffer frameBuffer) {
- try {
- return (TNonblockingTransport) fbTansportField.get(frameBuffer);
- } catch (IllegalAccessException e) {
- throw new IllegalStateException(e);
- }
- }
-
- @Override
- protected Runnable getRunnable(FrameBuffer frameBuffer) {
- return () -> {
-
- try {
- TNonblockingTransport transport = getTransport(frameBuffer);
-
- if (transport instanceof TNonblockingSocket tsock) {
- // This block of code makes the client address available to the server side code that
- // executes a RPC. It is made available for informational purposes.
- Socket sock = tsock.getSocketChannel().socket();
- TServerUtils.clientAddress
- .set(sock.getInetAddress().getHostAddress() + ":" + sock.getPort());
Review Comment:
This change needs to be in CustomThreadedSelectorServer.java?
--
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
Re: [PR] Remove Custom...Server.java classes and replace with Thrift implementation [accumulo]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on PR #6454:
URL: https://github.com/apache/accumulo/pull/6454#issuecomment-5376355092
I'm seeing similar things, with performance problems on a bunch of ITs on a private build server. I can't tell if they are related to these changes, or not.
--
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
Re: [PR] Remove Custom...Server.java classes and replace with Thrift implementation [accumulo]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #6454:
URL: https://github.com/apache/accumulo/pull/6454#discussion_r3762409812
##########
server/base/src/main/java/org/apache/accumulo/server/rpc/ThriftServerType.java:
##########
@@ -43,10 +45,13 @@ private ThriftServerType(String name) {
}
public static ThriftServerType get(String name) {
+ final Logger log = LoggerFactory.getLogger(ThriftServerType.class);
if (StringUtils.isBlank(name)) {
return getDefault();
}
+ // check added for backwards compatibility
if (name.trim().equalsIgnoreCase("custom_hs_ha")) {
+ log.warn("Server type custom_hs_ha is deprecated, use {}", ThriftServerType.HS_HA);
Review Comment:
I think it was implemented that way because that's what I suggested in my earlier code review comment at https://github.com/apache/accumulo/pull/6454#discussion_r3731881909, which I suggested because we generally handle use of deprecated stuff with a warning. I'm not sure why the comment thread was closed without confirming this in a response, though. Probably just an oversight. I'll mark it Unresolved and tag @Amemeda so they can see this and respond.
--
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
Re: [PR] Remove Custom...Server.java classes and replace with Thrift implementation [accumulo]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #6454:
URL: https://github.com/apache/accumulo/pull/6454#discussion_r3731881909
##########
server/base/src/main/java/org/apache/accumulo/server/rpc/ThriftServerType.java:
##########
@@ -46,6 +46,9 @@ public static ThriftServerType get(String name) {
if (StringUtils.isBlank(name)) {
return getDefault();
}
+ if (name.trim().equalsIgnoreCase("custom_hs_ha")) {
+ return ThriftServerType.HS_HA;
+ }
Review Comment:
Now that I see this added, I think it needs a brief comment saying that it's for backwards compatibility, and also probably a warning log message saying that this type is deprecated.
--
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
Re: [PR] Remove Custom...Server.java classes and replace with Thrift implementation [accumulo]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on code in PR #6454:
URL: https://github.com/apache/accumulo/pull/6454#discussion_r3731893830
##########
server/base/src/main/java/org/apache/accumulo/server/rpc/TServerUtils.java:
##########
@@ -225,7 +228,21 @@ private static ServerAddress createNonBlockingServer(HostAndPort address, TProce
address = HostAndPort.fromParts(address.getHost(), transport.getPort());
}
- return new ServerAddress(new CustomNonBlockingServer(options), address);
+ final THsHaServer server = new THsHaServer(options) {
+ @Override
+ public void stop() {
+ super.stop();
+ try {
+ getInvoker().shutdownNow();
+ } catch (Exception e) {
+ log.error("Unable to call shutdownNow", e);
+ }
+ }
+ };
Review Comment:
It turns out there's an existing gracefulShutdown method in the parent class, but it doesn't look like it works correctly, as it doesn't check the return status correctly. This code avoids that buggy code, and stops it directly, which is better. I'm probably going to have to submit an upstream bug about the graceful shutdown methods.
--
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
Re: [PR] Removing CustomThreadedSelectorServer.java [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #6454:
URL: https://github.com/apache/accumulo/pull/6454#discussion_r3508583112
##########
server/base/src/main/java/org/apache/accumulo/server/rpc/CustomThreadedSelectorServer.java:
##########
@@ -1,74 +0,0 @@
-/*
- * Licensed to the Apache Software Foundation (ASF) under one
- * or more contributor license agreements. See the NOTICE file
- * distributed with this work for additional information
- * regarding copyright ownership. The ASF licenses this file
- * to you under the Apache License, Version 2.0 (the
- * "License"); you may not use this file except in compliance
- * with the License. You may obtain a copy of the License at
- *
- * https://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing,
- * software distributed under the License is distributed on an
- * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
- * KIND, either express or implied. See the License for the
- * specific language governing permissions and limitations
- * under the License.
- */
-package org.apache.accumulo.server.rpc;
-
-import java.lang.reflect.Field;
-import java.net.Socket;
-
-import org.apache.thrift.server.TThreadedSelectorServer;
-import org.apache.thrift.transport.TNonblockingSocket;
-import org.apache.thrift.transport.TNonblockingTransport;
-import org.slf4j.LoggerFactory;
-
-public class CustomThreadedSelectorServer extends TThreadedSelectorServer {
-
- private final Field fbTansportField;
-
- public CustomThreadedSelectorServer(Args args) {
- super(args);
-
- try {
- fbTansportField = FrameBuffer.class.getDeclaredField("trans_");
- fbTansportField.setAccessible(true);
- } catch (SecurityException | NoSuchFieldException e) {
- throw new IllegalStateException("Failed to access required field in Thrift code.", e);
- }
- }
-
- private TNonblockingTransport getTransport(FrameBuffer frameBuffer) {
- try {
- return (TNonblockingTransport) fbTansportField.get(frameBuffer);
- } catch (IllegalAccessException e) {
- throw new IllegalStateException(e);
- }
- }
-
- @Override
- protected Runnable getRunnable(FrameBuffer frameBuffer) {
- return () -> {
-
- try {
- TNonblockingTransport transport = getTransport(frameBuffer);
-
- if (transport instanceof TNonblockingSocket tsock) {
- // This block of code makes the client address available to the server side code that
- // executes a RPC. It is made available for informational purposes.
- Socket sock = tsock.getSocketChannel().socket();
- TServerUtils.clientAddress
- .set(sock.getInetAddress().getHostAddress() + ":" + sock.getPort());
Review Comment:
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.
--
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
Re: [PR] Remove Custom...Server.java classes and replace with Thrift implementation [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on PR #6454:
URL: https://github.com/apache/accumulo/pull/6454#issuecomment-5368832047
I kicked off an IT build on this branch a few days ago. Looking at the results the following ITs failed or were flaky:
KerberosIT
SslWithClientAuthIT
KerberosRenewalIT
SslIT
ThriftMaxFrameSizeIT
I kicked off another run, just in case this was an issue with the server. Suggest merging the head of main into this branch and re-running them locally.
--
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
Re: [PR] Remove Custom...Server.java classes and replace with Thrift implementation [accumulo]
Posted by "ctubbsii (via GitHub)" <gi...@apache.org>.
ctubbsii commented on PR #6454:
URL: https://github.com/apache/accumulo/pull/6454#issuecomment-5272138330
It looks like some tests aren't working with these changes. I haven't dug into why yet.
--
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
Re: [PR] Remove Custom...Server.java classes and replace with Thrift implementation [accumulo]
Posted by "Amemeda (via GitHub)" <gi...@apache.org>.
Amemeda commented on PR #6454:
URL: https://github.com/apache/accumulo/pull/6454#issuecomment-5272196127
Not sure if its related, but #6491 is also timing out on a mini cluster test
--
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
Re: [PR] Removing CustomThreadedSelectorServer.java [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #6454:
URL: https://github.com/apache/accumulo/pull/6454#discussion_r3551511188
##########
server/base/src/main/java/org/apache/accumulo/server/rpc/ThriftServerEventHandler.java:
##########
@@ -0,0 +1,66 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.server.rpc;
+
+import java.net.SocketAddress;
+
+import org.apache.thrift.protocol.TProtocol;
+import org.apache.thrift.server.ServerContext;
+import org.apache.thrift.server.TServerEventHandler;
+import org.apache.thrift.transport.TTransport;
+
+public class ThriftServerEventHandler implements TServerEventHandler {
+
+ public static class ThriftServerContext implements ServerContext {
+
+ @Override
+ public <T> T unwrap(Class<T> iface) {
+ return null;
Review Comment:
I think we should throw an `UnsupportedOperationException` here. According to the ServerContext javadoc, this is the correct action.
https://github.com/apache/thrift/blob/master/lib/java/src/main/java/org/apache/thrift/server/ServerContext.java#L35
##########
server/base/src/main/java/org/apache/accumulo/server/rpc/ThriftServerEventHandler.java:
##########
@@ -0,0 +1,66 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.server.rpc;
+
+import java.net.SocketAddress;
+
+import org.apache.thrift.protocol.TProtocol;
+import org.apache.thrift.server.ServerContext;
+import org.apache.thrift.server.TServerEventHandler;
+import org.apache.thrift.transport.TTransport;
+
+public class ThriftServerEventHandler implements TServerEventHandler {
+
+ public static class ThriftServerContext implements ServerContext {
+
+ @Override
+ public <T> T unwrap(Class<T> iface) {
+ return null;
+ }
+
+ @Override
+ public boolean isWrapperFor(Class<?> iface) {
+ return false;
+ }
+
+ @Override
+ public void setRemoteAddress(SocketAddress remoteAddress) {
+ TServerUtils.clientAddress.set(remoteAddress.toString());
+ }
+
+ }
+
+ @Override
+ public void preServe() {}
+
+ @Override
+ public ServerContext createContext(TProtocol input, TProtocol output) {
+ return new ThriftServerContext();
Review Comment:
Based on the TServerEventHandler javadoc, and the fact that we are only manipulating a ThreadLocal, we can probably just return a reference to a static ThriftServerContext object. Suggest adding the following at the top of this class, then just `return context;` here.
```
private static final ThriftServerContext context = new ThriftServerContext();
```
https://github.com/apache/thrift/blob/master/lib/java/src/main/java/org/apache/thrift/server/TServerEventHandler.java
##########
server/base/src/main/java/org/apache/accumulo/server/rpc/ThriftServerEventHandler.java:
##########
@@ -0,0 +1,66 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.server.rpc;
+
+import java.net.SocketAddress;
+
+import org.apache.thrift.protocol.TProtocol;
+import org.apache.thrift.server.ServerContext;
+import org.apache.thrift.server.TServerEventHandler;
+import org.apache.thrift.transport.TTransport;
+
+public class ThriftServerEventHandler implements TServerEventHandler {
+
+ public static class ThriftServerContext implements ServerContext {
+
+ @Override
+ public <T> T unwrap(Class<T> iface) {
+ return null;
+ }
+
+ @Override
+ public boolean isWrapperFor(Class<?> iface) {
+ return false;
+ }
+
+ @Override
+ public void setRemoteAddress(SocketAddress remoteAddress) {
+ TServerUtils.clientAddress.set(remoteAddress.toString());
+ }
+
+ }
+
+ @Override
+ public void preServe() {}
+
+ @Override
+ public ServerContext createContext(TProtocol input, TProtocol output) {
+ return new ThriftServerContext();
+ }
+
+ @Override
+ public void deleteContext(ServerContext serverContext, TProtocol input, TProtocol output) {
+ TServerUtils.clientAddress.set("");
Review Comment:
It might make sense to add the following to ThriftServerContext and call it here. That way the calls to TServerUtils.clientAddress are localized to one class.
```
public void clear() {
TServerUtils.clientAddress.set("");
}
```
##########
server/base/src/main/java/org/apache/accumulo/server/rpc/TServerUtils.java:
##########
@@ -225,7 +228,10 @@ private static ServerAddress createNonBlockingServer(HostAndPort address, TProce
address = HostAndPort.fromParts(address.getHost(), transport.getPort());
}
- return new ServerAddress(new CustomNonBlockingServer(options), address);
+ CustomNonBlockingServer server = new CustomNonBlockingServer(options);
Review Comment:
It appears the CustomNonBlockingServer is also custom solely to get the client address. We might be able to remove this as well.
--
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
Re: [PR] Removing Custom...Server.java classes and replacing with Thrift implementation [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #6454:
URL: https://github.com/apache/accumulo/pull/6454#discussion_r3559759422
##########
server/base/src/main/java/org/apache/accumulo/server/rpc/TServerUtils.java:
##########
Review Comment:
I'm not sure that this is needed anymore (the method call and the ClientInfoProcessorFactory class). Look at other calls to 'processorFactory' to see how they are used in this class.
##########
server/base/src/main/java/org/apache/accumulo/server/rpc/ThriftServerEventHandler.java:
##########
@@ -0,0 +1,66 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.server.rpc;
+
+import java.net.SocketAddress;
+
+import org.apache.thrift.protocol.TProtocol;
+import org.apache.thrift.server.ServerContext;
+import org.apache.thrift.server.TServerEventHandler;
+import org.apache.thrift.transport.TTransport;
+
+public class ThriftServerEventHandler implements TServerEventHandler {
+
+ public static class ThriftServerContext implements ServerContext {
+
+ @Override
+ public <T> T unwrap(Class<T> iface) {
+ return null;
Review Comment:
This still needs to be done I think
--
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
Re: [PR] Remove Custom...Server.java classes and replace with Thrift implementation [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #6454:
URL: https://github.com/apache/accumulo/pull/6454#discussion_r3737701797
##########
server/base/src/main/java/org/apache/accumulo/server/rpc/ThriftServerType.java:
##########
@@ -43,10 +45,13 @@ private ThriftServerType(String name) {
}
public static ThriftServerType get(String name) {
+ final Logger log = LoggerFactory.getLogger(ThriftServerType.class);
if (StringUtils.isBlank(name)) {
return getDefault();
}
+ // check added for backwards compatibility
if (name.trim().equalsIgnoreCase("custom_hs_ha")) {
+ log.warn("Server type custom_hs_ha is deprecated, use {}", ThriftServerType.HS_HA);
Review Comment:
Did you resolve this comment because you confirmed that `warn` was correct?
--
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