Create tserver.group, add group to location information by dlmarion · Pull Request #3482 · apache/accumulo

dlmarion · 2023-06-12T14:29:46Z

This change includes:

Adding TSERV_GROUP_NAME property to add resource groups to TabletServers like has been done for Compactors and ScanServers
Modified TServerInstance to include the group in the location information.
Modified TabletMetadata to remove the session from the location columns colq and put all of the location information in the column value.
Modified MetricsUtil to add the servers resource group as a tag to all emitted metrics
Modified CachedTablet to use the TServerInstance instead of interned strings for the server and session.

1. Adding TSERV_GROUP_NAME property to add resource groups to TabletServers like has been done for Compactors and ScanServers 2. Modified TServerInstance to include the group in the location information. 3. Modified TabletMetadata to remove the session from the location columns colq and put all of the location information in the column value. 4. Modified MetricsUtil to add the servers resource group as a tag to all emitted metrics 5. Modified CachedTablet to use the TServerInstance instead of interned strings for the server and session.

dlmarion · 2023-06-12T14:30:41Z

This ended up being a bigger change than I expected. Follow-on work includes:

Rename compactor.queue
Update scripts for compactor.queue, addition of tserver.group
TabletServer resource group tests, to include balancers

dlmarion · 2023-06-12T14:34:45Z

-    mutation.put(getLocationFamily(location.getType()), location.getSession(),
-        location.getHostPort());
+    mutation.put(getLocationFamily(location.getType()), "",
+        location.getServerInstance().toString());


This may be one of the biggest changes in this PR. It removes the sessionId from the ColQ of the location columns in the metadata and puts the <host:port>#<sessionId>#<resourceGroup> in the Value. I was wondering why the sessionId was in the colq to begin with, that seems like it would allow more than one entry for the location type in the metadata table.

Associated with this change, I ended up removing a test for https://issues.apache.org/jira/browse/ACCUMULO-1248 as it no longer applied - the test would not work.

Not sure if session id applies here, but my impression is that session id was used to detect client ZooKeeper re-connections. If a ZooKeeper session failed (did not respond with ZooKeeper tick time), but then established a "new" connection - the change in session id was detected to signal that there is a possibility that information in ZooKeeper could have changed while the session was "off-line". This may be a ZooCache centered view, but maybe it applied here?

That's good information. I don't have the history to know why sessionId was in the column qualifier. It seemed like a bug to me as it would allow multiple current, future, or last entries.

@EdColeman - do you happen to know where the code is that detects a change in sessionId?

Edit: I think it's TabletMetadata.getTabletState(Set<TServerInstance>). That code should still work.

I was wondering why the sessionId was in the colq to begin with, that seems like it would allow more than one entry for the location type in the metadata table.

I do not know of a specific reason why it was structured that way. It did allow detection of certain bugs like if the manager set a location on a tablet that already had a location, but I don't think that is why it was structured that way. Using conditional mutations can help avoid these bugs instead of detecting them after the fact. We could take this a step further and push the current vs future designation into the value and have a single column for location.

keith-turner

Not finished looking at this. Plan to look at it some more tomorrow.

keith-turner · 2023-06-12T17:22:15Z

    this.session = session;
    this.hostPort = hostAndPort.toString();
-    this.hostPortSession = hostPort + "[" + session + "]";
+    this.group = group;


Is it expected that group can be null? If so is null expected to make it through serialization and deserialization?

No, the default value is ServiceDescriptor..DEFAULT_GROUP_NAME.

keith-turner · 2023-06-12T17:24:46Z

-      return tServerInstance.getHost();
-    }
-
-    public String getHostPort() {


Why were these methods removed?

Because they were redundant. The calling code can just call the method directly on the TServerInstance returned by getServerInstance().

ok, personally I liked some of these. Saw them as convenience methods that made common patterns a bit shorter. But I don't care that much if they are removed.

I agree that it makes the calling code shorter to read. However, I saw them as a potential source for a bug if for some reason someone changed them.

keith-turner · 2023-06-12T17:34:18Z

      return 0;
    }
-    return this.getHostPortSession().compareTo(other.getHostPortSession());
+    return this.getHostPortSessionGroup().compareTo(other.getHostPortSessionGroup());


Is it expected that the group would never change for an instance? If it did change it would be problematic to include it in compare, hashCode, and equals methods for tserver instance.

The group would be passed to the process as an argument (-o tserver.group=foo), so it is expected to be static for the life of the instance. If the argument is not supplied, then the default value is used (ServiceDescriptor..DEFAULT_GROUP_NAME = default).

keith-turner · 2023-06-12T17:43:33Z

        updateCache(locToCache, lcSession);
      }
+    } else {
+      log.warn("Metadata tablet not found for row: {}", metadataRow);


This case could happen when a metadata tablet is temporarily not hosted. Would not want to warn in that case.

Suggested change

log.warn("Metadata tablet not found for row: {}", metadataRow);

log.debug("Metadata tablet not found for row: {}", metadataRow);

keith-turner · 2023-06-12T18:21:51Z

      "1.3.5"),
+  TABLE_ASSIGNMENT_GROUP("table.assignment.group", ServiceDescriptor.DEFAULT_GROUP_NAME,
+      PropertyType.STRING,
+      "Tablets for this table will be assigned to TabletServers that have a corresponding"


This statement may not be true, it depends on the behavior of the balancer? Unless the balancer is limited to only assigning to tserver within the group?

We could limit balancing choices to a group, so a balancer could only chose from tservers within this group. That may require some changes to the SPI, not sure. Or this could be provided as information to the balancer, but it does not have to honor it. If the balancer does not have to honor it then would need to change the description.

Unless the balancer is limited to only assigning to tserver within the group?

I think we should do this given that there is a default resource group and the ability to create others.

keith-turner · 2023-06-12T19:36:58Z

+          te.setLocationOnce(val, LocationType.FUTURE, suppressLocationError);
          break;
        case LastLocationColumnFamily.STR_NAME:
-          te.last = Location.last(val, qual);


What is the benefit of pushing the tservers group into the metadata table for each tablet?

It would be needed by the Manager to know whether or not a table's tablets are balanced in the correct group.

The manager could get the group information about each tserver w/o it being stored in each tablet.

You may be correct, and it may already be done with the changes that I made to LiveTServerSet. I will investigate.

dlmarion · 2023-06-26T13:29:13Z

Closing in favor of the implementation in #3496

dlmarion requested a review from keith-turner June 12, 2023 14:29

dlmarion commented Jun 12, 2023

View reviewed changes

dlmarion linked an issue Jun 12, 2023 that may be closed by this pull request

Resource Groups: Create TabletServer property like sserver.group that can be used for assignment / balancing #3459

Closed

dlmarion self-assigned this Jun 12, 2023

Removed Interner, no longer referenced

8db080a

keith-turner reviewed Jun 12, 2023

View reviewed changes

Changes to TServerInstance based on PR feedback

eeb38b5

dlmarion mentioned this pull request Jun 14, 2023

Add TSERV_GROUP_NAME, ability to balance within tserver group #3496

Merged

dlmarion closed this Jun 26, 2023

dlmarion deleted the 3459-tserver-groups branch October 24, 2023 20:40

ctubbsii added this to the 4.0.0 milestone Jul 12, 2024

	log.warn("Metadata tablet not found for row: {}", metadataRow);
	log.debug("Metadata tablet not found for row: {}", metadataRow);

Conversation

dlmarion commented Jun 12, 2023

Uh oh!

dlmarion commented Jun 12, 2023

Uh oh!

dlmarion Jun 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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 Jun 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keith-turner left a comment

Choose a reason for hiding this comment

Uh oh!

keith-turner Jun 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

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!

keith-turner Jun 12, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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 Jun 26, 2023

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

dlmarion Jun 12, 2023 •

edited

Loading

dlmarion Jun 12, 2023 •

edited

Loading

keith-turner Jun 12, 2023 •

edited

Loading

keith-turner Jun 12, 2023 •

edited

Loading