Skip to content

Remove Custom...Server.java classes and replace with Thrift implementation - #6454

Open
Amemeda wants to merge 16 commits into
apache:mainfrom
Amemeda:cleanup
Open

Remove Custom...Server.java classes and replace with Thrift implementation#6454
Amemeda wants to merge 16 commits into
apache:mainfrom
Amemeda:cleanup

Conversation

@Amemeda

@Amemeda Amemeda commented Jun 30, 2026

Copy link
Copy Markdown
Contributor

Investigated relevance of a few custom tserver related java classes, deleted them, reworked their implementation with thrift methods

Result:

  • Deleted files CustomThreadedSelectorServer.java, CustomBlockingServer.java, ClientInfoProcessorFactory.java
  • Added file ThriftServerEventHandler.java
  • in TServerUtils.java, implementing method setServerEventHandler(new ThriftServerEventHandler()); to applicable tservers
  • removed CUSTOM from ThriftServerType.CUSTOM_HS_HA, updated usages

@Amemeda
Amemeda marked this pull request as ready for review June 30, 2026 16:03
@Amemeda

Amemeda commented Jun 30, 2026

Copy link
Copy Markdown
Contributor Author

@ctubbsii

@DomGarguilo DomGarguilo added this to the 4.0.0 milestone Jun 30, 2026
@DomGarguilo
DomGarguilo requested a review from ctubbsii June 30, 2026 21:04
@Amemeda
Amemeda requested a review from dlmarion July 8, 2026 16:23
Comment thread server/base/src/main/java/org/apache/accumulo/server/rpc/TServerUtils.java Outdated
@Amemeda Amemeda changed the title Removing CustomThreadedSelectorServer.java Removing Custom...Server.java classes and replacing with Thrift implementation Jul 9, 2026
@Amemeda
Amemeda requested a review from dlmarion July 10, 2026 14:19
Comment thread server/base/src/main/java/org/apache/accumulo/server/rpc/TServerUtils.java Outdated
@Amemeda
Amemeda requested a review from dlmarion July 13, 2026 15:09
@Amemeda Amemeda changed the title Removing Custom...Server.java classes and replacing with Thrift implementation Remove Custom...Server.java classes and replace with Thrift implementation Jul 16, 2026
@Amemeda
Amemeda requested a review from ctubbsii July 24, 2026 16:29

@ctubbsii ctubbsii left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I need to check this over again to see if there's anything else, but I did find one thing.

Comment thread server/base/src/main/java/org/apache/accumulo/server/rpc/TServerUtils.java Outdated
@dlmarion

Copy link
Copy Markdown
Contributor

I think ThriftServerType.CUSTOM_HS_HA needs to be renamed to remove CUSTOM_ from the enum name and value.

@ctubbsii ctubbsii left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, but a few thoughts:

  1. Still some room to clean up boilerplate to reduce redundant setting of the event handler
  2. I am very wary of relying on the ThreadLocal. I think code that uses the clientAddress should really make sure to grab it right after the initial RPC connection, and pass it along, so that way they know they are grabbing it from the same thread in which it is set; I skimmed all the times the thread local was accessed, and it seemed like it was only being done in the same thread where it is set, but it's hard to verify because in many places, we only use it when an exception occurs.
  3. Since the enum string was altered, backwards compatibility needs to be fixed if a user explicitly set "custom_hs_ha" in their config instead of relying on the defaults
  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.

Of these, 3 and 4 are the most important. 1 and 2 could be done as a follow on. Of those, 1 is the more important one for me, since its smaller and would simplify the utility class a bit, making the code slightly more maintainable (which is important for future work).

@Amemeda

Amemeda commented Aug 3, 2026

Copy link
Copy Markdown
Contributor Author
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,

image image

@Amemeda
Amemeda requested a review from ctubbsii August 4, 2026 16:54
Comment thread server/base/src/main/java/org/apache/accumulo/server/rpc/ThriftServerType.java Outdated
@Amemeda
Amemeda requested a review from ctubbsii August 7, 2026 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants