Use ServerId to represent a server address #5875

dlmarion · 2025-09-10T21:31:46Z

As part of #5775 ThriftTransportPool.getAnyCachedTransport was modified to perform a ZooKeeper lookup to get the ResourceGroup for a server because that information is not available on the ThriftTransportKey. ThriftTransportKey contains the HostAndPort only. The reason for this change is to make the ResourceGroup available for the ResourceGroupPredicate that is passed into ThriftTransportPool.getAnyCachedTransport so that the ZooKeeper lookup can be removed.

The server address is currently represented by either a String or a HostAndPort object in the code. Additionally the addresses are persisted in file paths in HDFS and in the root and metadata table. I opted to use the ServerId object as a replacement as it already contains the necessary information and is part of the public API, but making the change touched a lot of code.

Closes #5848

As part of apache#5775 ThriftTransportPool.getAnyCachedTransport was modified to perform a ZooKeeper lookup to get the ResourceGroup for a server because that information is not available on the ThriftTransportKey. ThriftTransportKey contains the HostAndPort only. The reason for this change is to make the ResourceGroup available for the ResourceGroupPredicate that is passed into `ThriftTransportPool.getAnyCachedTransport` so that the ZooKeeper lookup can be removed. The server address is currently represented by either a String or a HostAndPort object in the code. Additionally the addresses are persisted in file paths in HDFS and in the root and metadata table. I opted to use the ServerId object as a replacement as it already contains the necessary information and is part of the public API, but making the change touched a lot of code. Closes apache#5848

keith-turner · 2025-09-12T14:29:49Z

core/src/main/java/org/apache/accumulo/core/client/admin/servers/ServerId.java

+    MANAGER, MINI, MONITOR, GARBAGE_COLLECTOR, COMPACTOR, SCAN_SERVER, TABLET_SERVER;
+  }
+
+  public static record ServerIdInfo(String type, String resourceGroup, String host, int port) {


ServerIdInfo seems like its needed for implementation of caching and serialization, but maybe not intended to be in the public API? Could move it outside f the public API or make it package private if that is the case.

No idea if this makes sense or would be workable, but looking at this wonder if ServerId should be a record type itself w/ supporting static methods some that are public API and some that are not.

Yeah, I figured some of my changes to ServerId might need to be re-worked (this, as well as HostAndPort). I'm trying to get all of the ITs to pass ( I had 6 fail last night).

ServerIdInfo seems like its needed for implementation of caching and serialization, but maybe not intended to be in the public API?

I moved this and other internal implementation methods to a new class, ServerIdUtil, in fb6acbb.

No idea if this makes sense or would be workable, but looking at this wonder if ServerId should be a record

I looked at this, and I don't think it can be done right now due to the transient field hostPort.

keith-turner · 2025-09-12T14:34:09Z

core/src/main/java/org/apache/accumulo/core/client/admin/servers/ServerId.java

+  private transient HostAndPort hostPort;

-  public ServerId(Type type, ResourceGroupId resourceGroup, String host, int port) {
+  private ServerId(Type type, ResourceGroupId resourceGroup, String host, int port) {


If we could also get the session id added to this type then we would know everything about a server. Not a change for this PR, but I suspect this PR would make that easier to do. May be able to replace TServerInstance type w/ ServerId if that were done.

keith-turner · 2025-09-12T14:35:40Z

core/src/main/java/org/apache/accumulo/core/client/admin/servers/ServerId.java

        + ", port= " + port + "]";
  }

+  public String toWalFileName() {


This seems like an implementation thing that should be moved out of the public API, maybe to a static method somewhere.

I tried to break ServerId into a client interface and an Impl object, but it causes a ton of changes. I'm going to take your suggestion and create a utility class with static methods to get the implementation details out of the client class.

I moved this and other internal implementation methods to a new class, ServerIdUtil, in fb6acbb.

keith-turner · 2025-09-12T14:37:43Z

core/src/main/java/org/apache/accumulo/core/client/admin/servers/ServerId.java

    return port;
  }

+  public synchronized HostAndPort getHostPort() {


May not want to introduce the HostAndPort type into the public API.

We might need an API class and an internal class. There is a lot of server-side code that uses HostAndPort and we don't want to be constantly translating.

I removed HostAndPort from the ServerId public methods in 7f11006

keith-turner · 2025-09-12T14:51:33Z

core/src/main/java/org/apache/accumulo/core/spi/balancer/data/TabletServerId.java

 public interface TabletServerId extends Comparable<TabletServerId> {
-  String getHost();

-  int getPort();


If we had the session in the ServerId, then this whole type in the spi could go away.

TServerInstance could go away as well.

keith-turner · 2025-09-12T15:00:46Z

server/manager/src/main/java/org/apache/accumulo/manager/TabletGroupWatcher.java

-      Iterator<TServerInstance> find = tLists.destinations
-          .tailMap(new TServerInstance(tm.getSuspend().server, " ")).keySet().iterator();
+      Iterator<TServerInstance> find =
+          tLists.destinations.tailMap(tm.getSuspend().server).keySet().iterator();


Probably still need to create the TServerInstance w/ the empty session string. This code is trying to find any server where the host and port match, but the sesssion id does not matter. The empty session string sorts before all. This code needs to be refactored so it can be unit tested in another PR. Wonder if the existing code works as it depends heavily on how those sort, which could have changed after this code was written.

Suggested change

tLists.destinations.tailMap(tm.getSuspend().server).keySet().iterator();

tLists.destinations.tailMap(new TServerInstance(tm.getSuspend().server, " ")).keySet().iterator();

started working on a little unit test for this code

I think we should change the code to use zero when the session is unknown. I think I have done that in this branch, but maybe not in all places.

I opened #5885

I think we should change the code to use zero when the session is unknown

hmm, I don't know if the session can be negative.

Looked into handling of negative and zero for that code, it sorts using unsigned hex so zero would be minimum even if there are negative numbers. In #5885 updated the impl to use zero and the test use a negative session and it all works. The test should catch any changes in sorting of zero relative to negative values.

keith-turner · 2025-09-12T15:07:45Z

core/src/main/java/org/apache/accumulo/core/client/MutationsRejectedException.java

   *
   */
-  public Collection<String> getErrorServers() {
+  public Collection<ServerId> getErrorServers() {


This method is public API. Its in a dark corner of the API, could change it. May be easiest to just leave itas string.

Generally, my concern about leaving these types of things as String, is that there is not enough information to reconstitute a ServerId in the event that it's needed.

keith-turner · 2025-09-12T15:09:21Z

core/src/main/java/org/apache/accumulo/core/client/TimedOutException.java

  }

-  public Set<String> getTimedOutSevers() {
+  public Set<ServerId> getTimedOutSevers() {


Could leave this as string since this is a public API method

keith-turner · 2025-09-12T15:09:56Z

core/src/main/java/org/apache/accumulo/core/client/admin/Locations.java

   * @return A tablet server location in the form of {@code <host>:<port>}
   */
-  String getTabletLocation(TabletId tabletId);
+  ServerId getTabletLocation(TabletId tabletId);


Could leave this as string also since its public API

keith-turner · 2025-09-12T15:17:47Z

core/src/main/java/org/apache/accumulo/core/clientImpl/ConditionalWriterImpl.java

  }

-  private final Map<String,ServerQueue> serverQueues;
+  private final Map<ServerId,ServerQueue> serverQueues;


This is really nice, changing from string to ServerId. These changes can be tricky though because some of the map methods take Object so it will still compile when a string is passed. My IDE warns on these types of mismatches. Wonder if anything in the maven build will do that.

dlmarion · 2025-09-19T14:55:03Z

core/src/main/java/org/apache/accumulo/core/clientImpl/ServerIdUtil.java

+        "Invalid server id in wal file: " + name);
+    if (parts.length == 2) {
+      // return an uncached tserver object
+      return tserver(parts[0], Integer.parseInt(parts[1]));


This is called from LogEntry.validatedLogEntry which returns a LogEntry object. The log path is stored in the tablet metadata in LogColumnFamily. This code here handles the current (not new) serialization of the server being just host+port. Creating the ServerId object here assumes the default ResourceGroup because we don't have any further information. This assumption could be dangerous if the LogEntry's ResourceGroup is used for comparison purposes in code that uses LogEntry.

dlmarion · 2025-09-19T14:59:12Z

core/src/main/java/org/apache/accumulo/core/metadata/TServerInstance.java

+    // TODO: WAL marker serializations using the old format could present a
+    // problem. If we change the code here to handle the old format, then
+    // we have to make a guess at the resource group, which could affect
+    // callers of this method (GcWalsFilter, WalStateManager, LiveTServerSet)


This is another area of concern for dealing with serialized information.

dlmarion · 2025-09-19T18:52:16Z

#5899 renders the need for these changes obsolete. I'm going to leave this here in case we decide to use some of these changes w/r/t replacing server address (String) with ServerId.

keith-turner · 2025-09-19T23:28:43Z

#5899 renders the need for these changes obsolete. I'm going to leave this here in case we decide to use some of these changes w/r/t replacing server address (String) with ServerId.

Using a more specific type than string for that code would be nice.

dlmarion added this to the 4.0.0 milestone Sep 10, 2025

dlmarion self-assigned this Sep 10, 2025

dlmarion added 2 commits September 11, 2025 18:09

Update ITs

0a358b3

Merge branch 'main' into 5648-cached-transport-optimization

4039d38

keith-turner reviewed Sep 12, 2025

View reviewed changes

dlmarion added 6 commits September 12, 2025 18:25

Add resource group to zk path and wal file name

dad2c16

Merge branch 'main' into 5648-cached-transport-optimization

2ca5f4e

Removed HostAndPort from ServerId public API

7f11006

Merge branch 'main' into 5648-cached-transport-optimization

9f76bdf

Created ServerIdUtil

fb6acbb

Fixed some ITs, handle older server wal file name serializations

e342779

DomGarguilo mentioned this pull request Sep 18, 2025

Optimize filtering of cached transports for resource group #5848

Closed

dlmarion added 5 commits September 18, 2025 21:25

Fixed SuspendedTabletsIT

eb9f0df

Merge branch 'main' into 5648-cached-transport-optimization

345dc62

Merge branch 'main' into 5648-cached-transport-optimization

c550f28

Fix AdvertiseAndBindIT

20ad0e6

Update ManagerApiIT to make clear TServerInstance is expected

007b2dd

dlmarion commented Sep 19, 2025

View reviewed changes

dlmarion marked this pull request as ready for review September 19, 2025 15:01

dlmarion marked this pull request as draft September 19, 2025 16:20

	tLists.destinations.tailMap(tm.getSuspend().server).keySet().iterator();
	tLists.destinations.tailMap(new TServerInstance(tm.getSuspend().server, " ")).keySet().iterator();

Use ServerId to represent a server address #5875

Are you sure you want to change the base?

Use ServerId to represent a server address #5875

Uh oh!

Conversation

dlmarion commented Sep 10, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dlmarion commented Sep 19, 2025

Uh oh!

keith-turner commented Sep 19, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants