keith-turner commented on code in PR #3496:
URL: https://github.com/apache/accumulo/pull/3496#discussion_r1244068886
##########
core/src/main/java/org/apache/accumulo/core/conf/Property.java:
##########
@@ -1069,6 +1075,11 @@ public enum Property {
+ "also consider configuring the `" +
NoDeleteConstraint.class.getName() + "` "
+ "constraint.",
"2.0.0"),
+ TABLE_ASSIGNMENT_GROUP("table.assignment.group",
Constants.DEFAULT_RESOURCE_GROUP_NAME,
Review Comment:
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.
##########
core/src/main/java/org/apache/accumulo/core/spi/balancer/TableLoadBalancer.java:
##########
@@ -23,21 +23,38 @@
import java.util.HashMap;
import java.util.Map;
import java.util.Map.Entry;
+import java.util.Set;
+import java.util.SortedMap;
+import java.util.TreeMap;
+import org.apache.accumulo.core.Constants;
import org.apache.accumulo.core.classloader.ClassLoaderUtil;
import org.apache.accumulo.core.conf.Property;
import org.apache.accumulo.core.data.TableId;
import org.apache.accumulo.core.data.TabletId;
import org.apache.accumulo.core.manager.balancer.AssignmentParamsImpl;
import org.apache.accumulo.core.manager.balancer.BalanceParamsImpl;
+import org.apache.accumulo.core.spi.balancer.data.TServerStatus;
import org.apache.accumulo.core.spi.balancer.data.TabletMigration;
import org.apache.accumulo.core.spi.balancer.data.TabletServerId;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
/**
+ * TabletBalancer that balances Tablets for a Table using the TabletBalancer
defined by
+ * {@link Property#TABLE_LOAD_BALANCER}. This allows for different Tables to
specify different
+ * TabletBalancer classes.
+ * <p>
+ * Note that in versions prior to 4.0 this class would pass all known
TabletServers to the Table
+ * load balancers. In version 4.0 this changed with the introduction of
+ * {@link Property#TABLE_ASSIGNMENT_GROUP} such that this balancer now only
passes the TabletServers
Review Comment:
If using the table.custom prop these docs could have text like :
```
Set table.custom.assignment.group=<group name> to limit assignment of
tablets to tablet servers that were started with the named group.
```
##########
server/base/src/main/java/org/apache/accumulo/server/manager/LiveTServerSet.java:
##########
@@ -393,6 +404,12 @@ public synchronized Set<TServerInstance>
getCurrentServers() {
return new HashSet<>(currentInstances.keySet());
}
+ public synchronized Map<String,Set<TServerInstance>>
getCurrentServersGroups() {
+ Map<String,Set<TServerInstance>> copy = new HashMap<>();
Review Comment:
Its possible that depending on the uses 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.
##########
core/src/main/java/org/apache/accumulo/core/spi/balancer/TableLoadBalancer.java:
##########
@@ -106,6 +123,41 @@ protected TabletBalancer getBalancerForTable(TableId
tableId) {
return balancer;
}
+ private SortedMap<TabletServerId,TServerStatus> getCurrentSetForTable(
+ SortedMap<TabletServerId,TServerStatus> allTServers,
+ Map<String,Set<TabletServerId>> groupedTServers, String resourceGroup) {
+
+ String groupName =
Review Comment:
I don't think we should throw an exception when there are zero tablet
servers, this could be temporary situation that happens normally. Maybe log a
warning and do nothing.
Also I don't think we should fall back to some default when a user
configured group has no tservers in it, could log a warning and take no action.
##########
core/src/main/java/org/apache/accumulo/core/spi/balancer/TableLoadBalancer.java:
##########
@@ -114,8 +166,12 @@ public void getAssignments(AssignmentParameters params) {
.computeIfAbsent(tid.getTable(), k -> new HashMap<>()).put(tid,
lastTserver));
for (Entry<TableId,Map<TabletId,TabletServerId>> e :
groupedUnassigned.entrySet()) {
Map<TabletId,TabletServerId> newAssignments = new HashMap<>();
- getBalancerForTable(e.getKey()).getAssignments(
- new AssignmentParamsImpl(params.currentStatus(), e.getValue(),
newAssignments));
+ // 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()));
Review Comment:
It using the custom table prop, could do the following.
```suggestion
environment.getConfiguration(e.getKey()).getTableCustom("assignment.group");
```
##########
core/src/main/java/org/apache/accumulo/core/spi/balancer/TabletBalancer.java:
##########
@@ -67,6 +67,15 @@ interface AssignmentParameters {
* Assigns {@code tabletId} to {@code tabletServerId}.
*/
void addAssignment(TabletId tabletId, TabletServerId tabletServerId);
+
+ /**
+ * Balancers can use this mapping in conjunction with {@link
Property#TABLE_ASSIGNMENT_GROUP} to
+ * assign tablets to tablet servers within the corresponding resource group
+ *
+ * @return map of resource group name to set of TServerInstance objects
+ * @since 4.0.0
+ */
+ Map<String,Set<TabletServerId>> currentResourceGroups();
Review Comment:
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.
```suggestion
Map<String,Set<TabletServerId>> currentResourceGroups();
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
