Add TSERV_GROUP_NAME, ability to balance within tserver group by dlmarion · Pull Request #3496 · apache/accumulo

dlmarion · 2023-06-14T20:21:41Z

Added TSERV_GROUP_NAME property and refactored how groups are determined within the server processes. Modified LiveTServerSet to pull the group from the ServiceLockData and add it to the TServerInstance. Modified the TabletGroupWatcher to use the group information in the TServerInstance to determine if the tablet is currently hosted on a TServer that is not in the correct group

dlmarion · 2023-06-14T20:22:38Z

This replaces PR #3482 . It does not put the tserver group name into the metadata. There is likely more work to be done here w/r/t the balancers and tests. Putting up for early feedback on approach.

dlmarion · 2023-06-16T16:49:44Z

Follow-on tasks include:

Rename compactor.queue to compactor.group
Update scripts for compactor.queue name change, addition of tserver.group

dlmarion · 2023-06-26T13:30:53Z

Follow-on tasks completed in https://github.com/dlmarion/accumulo/tree/3496-followon-rename-compactor-queue. Will create PR for elasticity branch once this merged.

keith-turner

This is really neat. I made comments, those could be addressed as follow on issues.

keith-turner · 2023-06-27T16:59:07Z

          + "also consider configuring the `" + NoDeleteConstraint.class.getName() + "` "
          + "constraint.",
      "2.0.0"),
+  TABLE_ASSIGNMENT_GROUP("table.assignment.group", Constants.DEFAULT_RESOURCE_GROUP_NAME,


This is configuration for the TableLoadBalancer plugin, other configured balancer plugins may ignore this. Instead of defining it as a table property could define it as a custom property like table.custom.assignment.group and describe this prop in the documentation for the TableLoadBalancer plugin. This scopes the property to the plugin.

it's also used in TabletGroupWatcher.getAssignmentsFromBalancer to print a warning if a tablet is assigned to a tserver outside of its resource group.

it's also used in TabletGroupWatcher.getAssignmentsFromBalancer to print a warning if a tablet is assigned to a tserver outside of its resource group.

I don't think TGW should try to validate if a particular plugin is behaving properly.

If we scope the config to plugin, it could allow TableLoadBalancer to have more complex configuration that is tightly coupled to it. For example TableLoadBalancer could support the property table.custom.assignment.group or table.custom.assignment.rangedGroups where rangedGroups allows a user to set a json value that specifies something like the folllowing.

For tablets in range (<row1>,<row2>] use tablet server group X

For tablets in range (<row3>,<row4>] use tablet server group Y

For all other tablet use tablet server group Z

So one property or the other could be set, if both are set it throws an error.

So, it wasn't just the warning in TGW.getAssignmentsFromBalancer. There is also this:

https://github.com/apache/accumulo/pull/3496/files#diff-dd97054ac43737177293a8f3a9231f05f0e4bc5eb0a3b042cad3fb8ce31b6caaR303-R325

Does this check also need to run in the tablet mgmt iterator?

I don't think it can unless we pass the group information to the iterator. The group is not in the metadata anymore, only in ZK. The LiveTServerSet object in the Manager is the only thing that knows about the TServer groups

If needed we could pass the tserver group information to the tablet mgmt iterator, we already pass the live tserver set I think so this would be a similar amount of data. We could call the balancer plugins in the tablet mgmt iterator if needed also. The compaction code is calling compaction plugins in CompactionJobGenerator inside tablet mgtm iterator to make decisions.

It may be good to have an IT that does something like the following.

Configures table A to be assigned to tserver group Z

Verifies assignments are as expected

Changes config such that table A is assigned to tserver group X

Starts a tserver in group X

Waits for tablets to be assigned to group X

I removed the TABLE_ASSIGNMENT_GROUP property from Property and kept it in TableLoadBalancer in the commits today. There is a lot in this thread. I need to re-read it and make sure I got to everything.

I added a test that you suggested above. It uncovered some other issues with tables being deleted and how the TabletManagementIterator handles that. I think I have everything in this thread done.

keith-turner · 2023-06-27T17:04:32Z

+      // get the group of tservers for this table
+      SortedMap<TabletServerId,TServerStatus> groupedTServers = getCurrentSetForTable(
+          params.currentStatus(), params.currentResourceGroups(),
+          environment.getConfiguration(e.getKey()).get(Property.TABLE_ASSIGNMENT_GROUP.getKey()));


It using the custom table prop, could do the following.

Suggested change

environment.getConfiguration(e.getKey()).get(Property.TABLE_ASSIGNMENT_GROUP.getKey()));

environment.getConfiguration(e.getKey()).getTableCustom("assignment.group");

keith-turner · 2023-06-27T17:10:32Z

  }

+  public synchronized Map<String,Set<TServerInstance>> getCurrentServersGroups() {
+    Map<String,Set<TServerInstance>> copy = new HashMap<>();


Its possible that depending on the usage patterns that it may be more efficient to compute an immutable map of this info when it changes. The tserver does this for online tablets in https://github.com/apache/accumulo/blob/main/server/tserver/src/main/java/org/apache/accumulo/tserver/OnlineTablets.java

I don't know if this would be useful w/o analyzing the larger way in which this is used and created.

keith-turner · 2023-06-27T17:21:11Z

+     * @return map of resource group name to set of TServerInstance objects
+     * @since 4.0.0
+     */
+    Map<String,Set<TabletServerId>> currentResourceGroups();


This would be a follow on, we may want to introduce a type for the group names instead of using string. This would be something like TableId.

Suggested change

Map<String,Set<TabletServerId>> currentResourceGroups();

Map<String,Set<TabletServerId>> currentResourceGroups();

The suggestion looks the same to me, but I think I understand what you are saying.

I think I meant to put some type name in the suggestion. Must have gotten distracted by something while writing the comment.

keith-turner · 2023-07-10T21:03:53Z

+      log.trace("Table {} is set to use resource group: {}", e.getKey(), tableResourceGroup);
+      final Map<TabletId,TabletServerId> newAssignments = new HashMap<>();
+      // get the group of tservers for this table
+      final SortedMap<TabletServerId,TServerStatus> groupedTServers = getCurrentSetForTable(


This could be a follow on. Need to determine if we need to do anything to avoid recomputing this map on every call to getAssignments. Not sure if it matters, depends on how frequently the manager calls this.

keith-turner · 2023-07-10T21:23:49Z

+
+import com.google.common.net.HostAndPort;
+
+public class TabletServerResourceGroupBalanceIT extends SharedMiniClusterBase {


Some more test that could be useful.

Test balancing the root and metadata table to diff tserver groups.

Test two user tables that balance to diff groups to make sure that works as expected.

Test tablets within in a single table that balance to different groups.

Test tablets within in a single table that balance to different groups.

This was one of your earlier suggestions, but is not implemented at this time. This currently supports a table being assigned to a single resource group. I wonder if we should let users use this first and provide feedback on how they want it to operate. Also, they could extend TableLoadBalancer and do this for themselves.

Test two user tables that balance to diff groups to make sure that works as expected.

testBalancerWithResourceGroups does this. It creates one table in the default group and one table in the GROUP1 group. Then it validates that all of the first tables tablets are located in the tserver in the default group and that all of the second tables tablets are located in the tserver in the GROUP1 group.

Test balancing the root and metadata table to diff tserver groups.

Is this any different than the testBalancerWithResourceGroups test? Are these tables handled differently?

The root tablet is handled differently, metadata table is more likely to be like everything else. When I added an IT for compaction of root and metadata, I found the root tablet did not work. Had to add this :

accumulo/server/base/src/main/java/org/apache/accumulo/server/manager/state/ZooTabletStateStore.java

Lines 79 to 84 in 406552e

CompactionJobGenerator cjg = new CompactionJobGenerator(new ServiceEnvironmentImpl(ctx));

var jobs = cjg.generateJobs(tm,

EnumSet.of(CompactionKind.SYSTEM, CompactionKind.USER, CompactionKind.SELECTOR));

if (!jobs.isEmpty()) {

actions.add(ManagementAction.NEEDS_COMPACTING);

}

Created #3587

@keith-turner - I added balance tests for root and metadata table in 6ca1a88, and they work.

If the SPI supports the functionality, then it would be good to ensure it actually works.

So, the TabletBalancer.getResourceGroup api method works at the tablet level now. The TabletMetadata.getTabletState method returns TabletState.ASSIGNED_TO_WRONG_GROUP when a tablet is not hosted on a tablet server that is in the same group that TabletBalancer.getResourceGroup returns. TableLoadBalancer passes the set of TabletServers that correspond to the resource group to the tables load balancer for assignment and balance purposes. This allows, for example, the SimpleLoadBalancer to assign and balance within the subset of tablet servers.

I think to do what you are asking (assign and balance at the tablet level based on resource group assignment), the table level balancers would need to be modified to make decisions on a per-tablet basis.

I guess one way to resolve this is to remove TabletBalancer.getResourceGroup such that only TableLoadBalancer has this functionality. TabletMetadata.getTabletState would only check the resource group if the balancer is TableLoadBalancer.

Discussed the above with @keith-turner , I'm going to merge as-is and create a follow-on issue.

dlmarion added 6 commits June 15, 2023 11:54

Add new index to LiveTServerSet instead of modifying TServerInstance

bda0157

Merge branch 'elasticity' into 3459-tserver-groups-attempt2

0b5f646

wip

0f1752a

fix javadoc

119d468

Merge branch 'elasticity' into 3459-tserver-groups-attempt2

d4b71fa

Update property language

ec2f2dd

dlmarion marked this pull request as ready for review June 16, 2023 17:05

dlmarion added 3 commits June 21, 2023 13:37

Merge branch 'elasticity' into 3459-tserver-groups-attempt2

62ff480

Merge branch 'elasticity' into 3459-tserver-groups-attempt2

a7b1762

Added ResourceGroupBalanceIT, necessary changes for it to work

736b7d1

dlmarion mentioned this pull request Jun 23, 2023

Modified MiniAccumuloCluster configuration to include cluster config #3533

Merged

Merge branch 'elasticity' into 3459-tserver-groups-attempt2

741f8b1

dlmarion self-assigned this Jun 23, 2023

dlmarion mentioned this pull request Jun 26, 2023

Create tserver.group, add group to location information #3482

Closed

dlmarion requested a review from keith-turner June 26, 2023 13:32

ctubbsii linked an issue Jun 26, 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

keith-turner mentioned this pull request Jun 27, 2023

Decide default compactor group names and make supporting changes #3546

Closed

keith-turner approved these changes Jun 27, 2023

View reviewed changes

dlmarion added 3 commits June 27, 2023 18:15

Merge branch 'elasticity' into 3459-tserver-groups-attempt2

9f080e5

Merge branch 'elasticity' into 3459-tserver-groups-attempt2

c2bfffd

Working on moving TABLE_ASSIGNMENT_GROUP, need more tests

deca07c

dlmarion commented Jul 5, 2023

View reviewed changes

Comment thread core/src/main/java/org/apache/accumulo/core/spi/balancer/TableLoadBalancer.java Outdated

dlmarion added 3 commits July 5, 2023 21:08

Added no tserver test

db29813

Merge branch 'elasticity' into 3459-tserver-groups-attempt2

3cb511b

Added test that table property change causes migration

92c61dc

dlmarion requested a review from keith-turner July 6, 2023 20:42

Merge branch 'elasticity' into 3459-tserver-groups-attempt2

508727a

dlmarion mentioned this pull request Jul 10, 2023

Consider adding service name to compactor group #3547

Closed

keith-turner reviewed Jul 10, 2023

View reviewed changes

Comment thread core/src/main/java/org/apache/accumulo/core/spi/balancer/TabletBalancer.java

keith-turner reviewed Jul 10, 2023

View reviewed changes

Comment thread test/src/main/java/org/apache/accumulo/test/functional/TabletServerResourceGroupBalanceIT.java Outdated

keith-turner reviewed Jul 10, 2023

View reviewed changes

dlmarion added 5 commits July 11, 2023 13:23

Removed Thread.sleep from test

d0b0c4d

Modified TabletBalancer api to just take TabletId

a03bfb6

Added tests for root and metadata table

6ca1a88

fix formatting

15cd817

Add missing override

6f5d9cd

keith-turner approved these changes Jul 11, 2023

View reviewed changes

Comment thread core/src/main/java/org/apache/accumulo/core/metadata/schema/TabletMetadata.java Outdated

Remove spammy log

899cb4e

dlmarion merged commit cfd7c80 into apache:elasticity Jul 11, 2023

dlmarion deleted the 3459-tserver-groups-attempt2 branch July 11, 2023 21:13

dlmarion mentioned this pull request Jul 11, 2023

Resolve resource group usage in TableLoadBalancer #3590

Closed

ctubbsii added this to the 4.0.0 milestone Jul 12, 2024

	environment.getConfiguration(e.getKey()).get(Property.TABLE_ASSIGNMENT_GROUP.getKey()));
	environment.getConfiguration(e.getKey()).getTableCustom("assignment.group");

	Map<String,Set<TabletServerId>> currentResourceGroups();
	Map<String,Set<TabletServerId>> currentResourceGroups();


		import com.google.common.net.HostAndPort;

		public class TabletServerResourceGroupBalanceIT extends SharedMiniClusterBase {

	CompactionJobGenerator cjg = new CompactionJobGenerator(new ServiceEnvironmentImpl(ctx));
	var jobs = cjg.generateJobs(tm,
	EnumSet.of(CompactionKind.SYSTEM, CompactionKind.USER, CompactionKind.SELECTOR));
	if (!jobs.isEmpty()) {
	actions.add(ManagementAction.NEEDS_COMPACTING);
	}

Conversation

dlmarion commented Jun 14, 2023

Uh oh!

dlmarion commented Jun 14, 2023

Uh oh!

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

Uh oh!

Uh oh!

dlmarion commented Jun 26, 2023

Uh oh!

keith-turner left a comment

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 27, 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!

keith-turner Jun 27, 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!

keith-turner Jun 27, 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!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

keith-turner Jun 27, 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!

Uh oh!

Uh oh!

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!

dlmarion Jul 11, 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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dlmarion commented Jun 16, 2023 •

edited

Loading

keith-turner Jun 27, 2023 •

edited

Loading

keith-turner Jun 27, 2023 •

edited

Loading

keith-turner Jun 27, 2023 •

edited

Loading

keith-turner Jun 27, 2023 •

edited

Loading

dlmarion Jul 11, 2023 •

edited

Loading