keith-turner commented on code in PR #5749:
URL: https://github.com/apache/accumulo/pull/5749#discussion_r2241239640
##########
core/src/main/thrift/client.thrift:
##########
@@ -59,7 +59,8 @@ enum TableOperationExceptionType {
}
enum ConfigurationType {
- CURRENT
+ PROCESS
Review Comment:
These names are an improvement.
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/ResourceGroupOperationsImpl.java:
##########
@@ -0,0 +1,278 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.clientImpl;
+
+import static com.google.common.base.Preconditions.checkArgument;
+
+import java.time.Duration;
+import java.util.ConcurrentModificationException;
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Set;
+import java.util.function.Consumer;
+
+import org.apache.accumulo.core.Constants;
+import org.apache.accumulo.core.client.AccumuloException;
+import org.apache.accumulo.core.client.AccumuloSecurityException;
+import org.apache.accumulo.core.client.ResourceGroupNotFoundException;
+import org.apache.accumulo.core.client.admin.ResourceGroupOperations;
+import org.apache.accumulo.core.clientImpl.thrift.ConfigurationType;
+import org.apache.accumulo.core.clientImpl.thrift.TVersionedProperties;
+import
org.apache.accumulo.core.clientImpl.thrift.ThriftResourceGroupNotExistsException;
+import org.apache.accumulo.core.conf.DeprecatedPropertyUtil;
+import org.apache.accumulo.core.data.ResourceGroupId;
+import org.apache.accumulo.core.rpc.clients.ThriftClientTypes;
+import org.apache.accumulo.core.trace.TraceUtil;
+import org.apache.accumulo.core.util.LocalityGroupUtil;
+import
org.apache.accumulo.core.util.LocalityGroupUtil.LocalityGroupConfigurationError;
+import org.apache.accumulo.core.util.Retry;
+import org.slf4j.LoggerFactory;
+
+public class ResourceGroupOperationsImpl implements ResourceGroupOperations {
+
+ private final ClientContext context;
+
+ public ResourceGroupOperationsImpl(ClientContext context) {
+ checkArgument(context != null, "context is null");
+ this.context = context;
+ }
+
+ @Override
+ public boolean exists(String group) {
+ ResourceGroupId rg = ResourceGroupId.of(group);
+ return context.getZooCache().get(Constants.ZRESOURCEGROUPS + "/" +
rg.canonical()) != null;
+ }
+
+ @Override
+ public Set<ResourceGroupId> list() {
+ Set<ResourceGroupId> groups = new HashSet<>();
+ context.getZooCache().getChildren(Constants.ZRESOURCEGROUPS)
+ .forEach(c -> groups.add(ResourceGroupId.of(c)));
+ return Set.copyOf(groups);
+ }
+
+ @Override
+ public void create(ResourceGroupId group) throws AccumuloException,
AccumuloSecurityException {
+ ThriftClientTypes.MANAGER.executeVoid(context, client -> client
+ .createResourceGroupNode(TraceUtil.traceInfo(), context.rpcCreds(),
group.canonical()));
+ }
+
+ @Override
+ public Map<String,String> getConfiguration(ResourceGroupId group)
+ throws AccumuloException, AccumuloSecurityException,
ResourceGroupNotFoundException {
+ Map<String,String> config = new HashMap<>();
+ config.putAll(ThriftClientTypes.CLIENT.execute(context, client -> client
+ .getConfiguration(TraceUtil.traceInfo(), context.rpcCreds(),
ConfigurationType.PROCESS)));
Review Comment:
Seems like this should be system.
```suggestion
.getConfiguration(TraceUtil.traceInfo(), context.rpcCreds(),
ConfigurationType.SYSTEM)));
```
##########
server/base/src/main/java/org/apache/accumulo/server/conf/ResourceGroupConfiguration.java:
##########
@@ -0,0 +1,70 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.server.conf;
+
+import org.apache.accumulo.core.conf.Property;
+import org.apache.accumulo.server.ServerContext;
+import org.apache.accumulo.server.conf.store.ResourceGroupPropKey;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class ResourceGroupConfiguration extends ZooBasedConfiguration {
+
+ private static final Logger LOG =
LoggerFactory.getLogger(ResourceGroupConfiguration.class);
+
+ private final RuntimeFixedProperties runtimeFixedProps;
+
+ public ResourceGroupConfiguration(ServerContext context,
ResourceGroupPropKey rgPropKey,
+ SystemConfiguration parent) {
+ super(LOG, context, rgPropKey, parent);
+ runtimeFixedProps = parent.getRuntimeFixedProperties();
Review Comment:
Does this mean that runtime fixed props can not be set at the RG level? Or
if they can be set we will not be able to read them? Seems like for these props
it will use system config value.
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/ClientContext.java:
##########
@@ -1277,10 +1288,18 @@ private static Set<String>
createPersistentWatcherPaths() {
for (String path : Set.of(Constants.ZCOMPACTORS, Constants.ZDEADTSERVERS,
Constants.ZGC_LOCK,
Constants.ZMANAGER_LOCK, Constants.ZMINI_LOCK, Constants.ZMONITOR_LOCK,
Constants.ZNAMESPACES, Constants.ZRECOVERY, Constants.ZSSERVERS,
Constants.ZTABLES,
- Constants.ZTSERVERS, Constants.ZUSERS, RootTable.ZROOT_TABLET,
Constants.ZTEST_LOCK)) {
+ Constants.ZTSERVERS, Constants.ZUSERS, RootTable.ZROOT_TABLET,
Constants.ZTEST_LOCK,
+ Constants.ZRESOURCEGROUPS)) {
pathsToWatch.add(path);
}
return pathsToWatch;
}
+ public ResourceGroupId getResourceGroupId(String rg) {
Review Comment:
This method is only used in ClientServiceHandler, could move it there to
simplify ClientContext
##########
test/src/main/java/org/apache/accumulo/test/conf/ResourceGroupConfigIT.java:
##########
@@ -0,0 +1,202 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.test.conf;
+
+import static org.apache.accumulo.harness.AccumuloITBase.MINI_CLUSTER_ONLY;
+import static org.apache.accumulo.harness.AccumuloITBase.SUNNY_DAY;
+import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertThrows;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+
+import java.time.Duration;
+import java.util.Map;
+import java.util.Set;
+import java.util.function.Consumer;
+
+import org.apache.accumulo.core.Constants;
+import org.apache.accumulo.core.client.Accumulo;
+import org.apache.accumulo.core.client.AccumuloException;
+import org.apache.accumulo.core.client.AccumuloSecurityException;
+import org.apache.accumulo.core.client.ResourceGroupNotFoundException;
+import org.apache.accumulo.core.client.admin.InstanceOperations;
+import org.apache.accumulo.core.client.admin.ResourceGroupOperations;
+import org.apache.accumulo.core.clientImpl.ClientContext;
+import org.apache.accumulo.core.conf.Property;
+import org.apache.accumulo.core.data.ResourceGroupId;
+import org.apache.accumulo.core.fate.zookeeper.ZooReaderWriter;
+import org.apache.accumulo.core.lock.ServiceLockPaths.AddressSelector;
+import org.apache.accumulo.core.lock.ServiceLockPaths.ServiceLockPath;
+import org.apache.accumulo.core.rpc.clients.TServerClient;
+import org.apache.accumulo.harness.MiniClusterConfigurationCallback;
+import org.apache.accumulo.harness.SharedMiniClusterBase;
+import org.apache.accumulo.miniclusterImpl.MiniAccumuloConfigImpl;
+import org.apache.accumulo.server.conf.store.ResourceGroupPropKey;
+import org.apache.accumulo.test.util.Wait;
+import org.apache.hadoop.conf.Configuration;
+import org.junit.jupiter.api.AfterAll;
+import org.junit.jupiter.api.BeforeAll;
+import org.junit.jupiter.api.Tag;
+import org.junit.jupiter.api.Test;
+
+@Tag(MINI_CLUSTER_ONLY)
+@Tag(SUNNY_DAY)
+public class ResourceGroupConfigIT extends SharedMiniClusterBase {
+
+ private static final String RG = "ResourceGroupTest";
+
+ @Override
+ protected Duration defaultTimeout() {
+ return Duration.ofMinutes(1);
+ }
+
+ @BeforeAll
+ public static void setup() throws Exception {
+ SharedMiniClusterBase.startMiniClusterWithConfig(new
MiniClusterConfigurationCallback() {
+ @Override
+ public void configureMiniCluster(MiniAccumuloConfigImpl cfg,
Configuration coreSite) {
+ cfg.getClusterServerConfiguration().setNumDefaultCompactors(1);
+ cfg.getClusterServerConfiguration().setNumDefaultScanServers(1);
+ cfg.getClusterServerConfiguration().setNumDefaultTabletServers(1);
+ }
+ });
+ }
+
+ @AfterAll
+ public static void teardown() {
+ SharedMiniClusterBase.stopMiniCluster();
+ }
+
+ @Test
+ public void testConfiguration() throws Exception {
+
+ final ResourceGroupId rgid = ResourceGroupId.of(RG);
+ final ResourceGroupPropKey rgpk = ResourceGroupPropKey.of(rgid);
+
+ try (var client = Accumulo.newClient().from(getClientProps()).build()) {
+ @SuppressWarnings("resource")
+ final ClientContext cc = (ClientContext) client;
+ final InstanceOperations iops = client.instanceOperations();
+ final ResourceGroupOperations rgOps = client.resourceGroupOperations();
+ final ZooReaderWriter zrw =
getCluster().getServerContext().getZooSession().asReaderWriter();
+
+ // Default node created in instance init
+ assertTrue(zrw.exists(ResourceGroupPropKey.DEFAULT.getPath()));
+
+ assertFalse(zrw.exists(Constants.ZRESOURCEGROUPS + "/" +
rgid.canonical()));
+ // This will get created by mini, but doing it here manually for testing.
+ rgOps.create(rgid);
+ assertTrue(zrw.exists(rgpk.getPath()));
+ assertTrue(rgOps.getProperties(rgid).isEmpty());
+
+ rgOps.setProperty(rgid, Property.COMPACTION_WARN_TIME.getKey(), "1m");
+
+ // Start the processes in the resource group
+
getCluster().getConfig().getClusterServerConfiguration().addCompactorResourceGroup(RG,
1);
+
getCluster().getConfig().getClusterServerConfiguration().addScanServerResourceGroup(RG,
1);
+
getCluster().getConfig().getClusterServerConfiguration().addTabletServerResourceGroup(RG,
1);
+ getCluster().start();
+
+ Wait.waitFor(() -> cc.getServerPaths()
+ .getCompactor(rg -> rg.equals(rgid), AddressSelector.all(),
true).size() == 1);
+ Wait.waitFor(() -> cc.getServerPaths()
+ .getScanServer(rg -> rg.equals(rgid), AddressSelector.all(),
true).size() == 1);
+ Wait.waitFor(() -> cc.getServerPaths()
+ .getTabletServer(rg -> rg.equals(rgid), AddressSelector.all(),
true).size() == 1);
+
+ checkProperty(iops, rgOps,
+ cc.getServerPaths().getCompactor(rg ->
rg.equals(ResourceGroupId.DEFAULT),
+ AddressSelector.all(), true),
+ ResourceGroupId.DEFAULT, Property.COMPACTION_WARN_TIME,
+ Property.COMPACTION_WARN_TIME.getDefaultValue(),
+ Property.COMPACTION_WARN_TIME.getDefaultValue());
+
+ checkProperty(iops, rgOps,
+ cc.getServerPaths().getScanServer(rg ->
rg.equals(ResourceGroupId.DEFAULT),
+ AddressSelector.all(), true),
+ ResourceGroupId.DEFAULT, Property.COMPACTION_WARN_TIME,
+ Property.COMPACTION_WARN_TIME.getDefaultValue(),
+ Property.COMPACTION_WARN_TIME.getDefaultValue());
+
+ checkProperty(iops, rgOps,
+ cc.getServerPaths().getTabletServer(rg ->
rg.equals(ResourceGroupId.DEFAULT),
+ AddressSelector.all(), true),
+ ResourceGroupId.DEFAULT, Property.COMPACTION_WARN_TIME,
+ Property.COMPACTION_WARN_TIME.getDefaultValue(),
+ Property.COMPACTION_WARN_TIME.getDefaultValue());
+
+ checkProperty(iops, rgOps,
+ cc.getServerPaths().getCompactor(rg -> rg.equals(rgid),
AddressSelector.all(), true),
+ rgid, Property.COMPACTION_WARN_TIME, "1m",
+ Property.COMPACTION_WARN_TIME.getDefaultValue());
+
+ checkProperty(iops, rgOps,
+ cc.getServerPaths().getScanServer(rg -> rg.equals(rgid),
AddressSelector.all(), true),
+ rgid, Property.COMPACTION_WARN_TIME, "1m",
+ Property.COMPACTION_WARN_TIME.getDefaultValue());
+
+ checkProperty(iops, rgOps,
+ cc.getServerPaths().getTabletServer(rg -> rg.equals(rgid),
AddressSelector.all(), true),
+ rgid, Property.COMPACTION_WARN_TIME, "1m",
+ Property.COMPACTION_WARN_TIME.getDefaultValue());
+
+ // test error cases
+ ResourceGroupId invalid = ResourceGroupId.of("INVALID");
+ Consumer<Map<String,String>> consumer = (m) -> {};
+ assertThrows(ResourceGroupNotFoundException.class, () ->
rgOps.getConfiguration(invalid));
+ assertThrows(ResourceGroupNotFoundException.class, () ->
rgOps.getProperties(invalid));
+ assertThrows(ResourceGroupNotFoundException.class,
+ () -> rgOps.setProperty(invalid,
Property.COMPACTION_WARN_TIME.getKey(), "1m"));
+ assertThrows(ResourceGroupNotFoundException.class,
+ () -> rgOps.modifyProperties(invalid, consumer));
+ assertThrows(ResourceGroupNotFoundException.class,
+ () -> rgOps.removeProperty(invalid,
Property.COMPACTION_WARN_TIME.getKey()));
+ assertThrows(ResourceGroupNotFoundException.class, () ->
rgOps.remove(invalid));
+
+ rgOps.remove(rgid);
+ assertFalse(zrw.exists(rgpk.getPath()));
+ assertFalse(zrw.exists(rgpk.getPath()));
+ assertFalse(zrw.exists(Constants.ZRESOURCEGROUPS + "/" +
rgid.canonical()));
+
+ }
+
+ }
+
Review Comment:
This could be a follow on, should add a test that checks permissions. Like
create a user that does not have the permission to create/remove RGs and try
making the calls w/ that user. Can assert that it fails and we get the correct
exception.
##########
core/src/main/java/org/apache/accumulo/core/data/ResourceGroupId.java:
##########
@@ -18,15 +18,34 @@
*/
package org.apache.accumulo.core.data;
+import java.util.List;
+import java.util.regex.Pattern;
+
import org.apache.accumulo.core.Constants;
-import org.apache.accumulo.core.conf.cluster.ClusterConfigParser;
import org.apache.accumulo.core.util.cache.Caches;
import org.apache.accumulo.core.util.cache.Caches.CacheName;
import com.github.benmanes.caffeine.cache.Cache;
public class ResourceGroupId extends AbstractId<ResourceGroupId> {
+ private static final Pattern GROUP_NAME_PATTERN =
Pattern.compile("^[a-zA-Z]+(_?[a-zA-Z0-9])*$");
+
+ public static void validateGroupName(ResourceGroupId rgid) {
+ if (!GROUP_NAME_PATTERN.matcher(rgid.canonical()).matches()) {
+ throw new IllegalArgumentException(
+ "Group name: " + rgid.canonical() + " contains invalid characters");
+ }
+ }
+
+ public static void validateGroupNames(List<String> names) {
+ for (String name : names) {
+ if (!GROUP_NAME_PATTERN.matcher(name).matches()) {
+ throw new RuntimeException("Group name: " + name + " contains invalid
characters");
+ }
+ }
+ }
Review Comment:
These will be in the public API, should they be? The first method seems
like a util method for the constructor to validate. Maybe the 2nd method could
be moved to ClusterConfigParser? If not and we do want it in the public API
then maybe it should throw IllegalArgumentException
##########
server/base/src/main/java/org/apache/accumulo/server/util/ResourceGroupPropUtil.java:
##########
@@ -0,0 +1,79 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.server.util;
+
+import org.apache.accumulo.core.conf.DeprecatedPropertyUtil;
+import org.apache.accumulo.core.conf.Property;
+import org.apache.accumulo.core.conf.PropertyType;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class ResourceGroupPropUtil {
+
+ private static final Logger LOG =
LoggerFactory.getLogger(ResourceGroupPropUtil.class);
+
+ public static String validateResourceGroupProperty(String property, String
value) {
+ // Retrieve the replacement name for this property, if there is one.
+ // Do this before we check if the name is a valid zookeeper name.
+ final var original = property;
+ property = DeprecatedPropertyUtil.getReplacementName(property,
+ (log, replacement) -> log.warn("{} was deprecated and will be removed
in a future release;"
+ + " setting its replacement {} instead", original, replacement));
+
+ if (Property.isValidResourceGroupPropertyKey(property)) {
+
+ if (!Property.isValidProperty(property, value)) {
+ IllegalArgumentException iae = new IllegalArgumentException(
+ "Property " + property + " with value: " + value + " is not
valid");
+ LOG.trace("Encountered error setting zookeeper property", iae);
+ throw iae;
+ }
+
+ // Find the property taking prefix into account
+ Property foundProp = null;
+ for (Property prop : Property.values()) {
+ if ((prop.getType() == PropertyType.PREFIX &&
property.startsWith(prop.getKey()))
+ || prop.getKey().equals(property)) {
+ foundProp = prop;
+ break;
+ }
+ }
+
+ if (foundProp == null || (foundProp.getType() != PropertyType.PREFIX
+ && !foundProp.getType().isValidFormat(value))) {
+ IllegalArgumentException iae = new IllegalArgumentException(
+ "Ignoring property " + property + " it is either null or in an
invalid format");
+ LOG.trace("Attempted to set zookeeper property. Value is either null
or invalid", iae);
+ throw iae;
+ }
Review Comment:
Not completely sure, but this code may be redundant w/ what
`Property.isValidProperty()` is doing a few lines up. That method will check
for valid prefixes also.
```suggestion
```
##########
server/base/src/main/java/org/apache/accumulo/server/conf/cluster/ClusterConfigParser.java:
##########
@@ -225,24 +243,42 @@ public static void
outputShellVariables(Map<String,String> config, PrintStream o
@SuppressFBWarnings(value = "PATH_TRAVERSAL_IN",
justification = "Path provided for output file is intentional")
public static void main(String[] args) throws IOException {
- if (args == null || args.length < 1 || args.length > 2) {
- System.err.println("Usage: ClusterConfigParser <configFile>
[<outputFile>]");
+ if (args == null || args.length < 1 || args.length > 3) {
+ System.err.println(
+ "Usage: ClusterConfigParser <createMissingResourceGroups>
<configFile> [<outputFile>]");
System.exit(1);
}
- try {
- if (args.length == 2) {
- // Write to a file instead of System.out if provided as an argument
- try (OutputStream os = Files.newOutputStream(Path.of(args[1]));
- PrintStream out = new PrintStream(os)) {
- outputShellVariables(parseConfiguration(Path.of(args[0])), out);
+ if (siteConf == null) {
+ siteConf = SiteConfiguration.auto();
+ }
+ try (var context = new ServerContext(siteConf)) {
+ // Login as the server on secure HDFS
+ if (siteConf.getBoolean(Property.INSTANCE_RPC_SASL_ENABLED)) {
+ SecurityUtil.serverLogin(siteConf);
+ }
+ final Set<String> zkGroups = new HashSet<>();
+ context.resourceGroupOperations().list().forEach(rg ->
zkGroups.add(rg.canonical()));
+ if (!zkGroups.contains(ResourceGroupId.DEFAULT.canonical())) {
+ throw new IllegalStateException("Default resource group not found in
ZooKeeper");
+ }
Review Comment:
Maybe this should be pushed into the list() operation.
##########
core/src/main/java/org/apache/accumulo/core/conf/Property.java:
##########
@@ -1668,6 +1668,13 @@ public static boolean isValidZooPropertyKey(String key) {
||
key.equals(Property.GENERAL_FILE_NAME_ALLOCATION_BATCH_SIZE_MAX.getKey());
}
+ public static boolean isValidResourceGroupPropertyKey(String property) {
+ return property.startsWith(Property.GENERAL_PREFIX.getKey())
Review Comment:
Could also add RPC prefix.
##########
server/base/src/main/java/org/apache/accumulo/server/conf/cluster/ClusterConfigParser.java:
##########
@@ -147,11 +141,32 @@ private static List<String> parseGroup(Map<String,String>
config, String prefix)
throw new IllegalArgumentException("Malformed configuration, has too
many levels: " + k);
}
}).sorted().distinct().collect(Collectors.toList());
- validateGroupNames(groups);
+ ResourceGroupId.validateGroupNames(groups);
return groups;
}
- public static void outputShellVariables(Map<String,String> config,
PrintStream out) {
+ private static void validateConfiguredGroups(final ServerContext ctx, final
Set<String> zkGroups,
+ final List<String> configuredGroups, boolean createMissingRG) {
+ for (String cg : configuredGroups) {
+ if (!zkGroups.contains(cg)) {
+ if (createMissingRG) {
+ try {
+ ResourceGroupPropKey.of(ResourceGroupId.of(cg))
+ .createZNode(ctx.getZooSession().asReaderWriter());
Review Comment:
Can the public API be used here? Its nice to pull the code that creates an
RG into a single place in case how it is done changes in the future.
```suggestion
ctx.resourceGroupOperations().create(ResourceGroupId.of(cg));
```
##########
core/src/main/java/org/apache/accumulo/core/client/admin/ResourceGroupOperations.java:
##########
@@ -0,0 +1,217 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.accumulo.core.client.admin;
+
+import java.util.Map;
+import java.util.Set;
+import java.util.function.Consumer;
+
+import org.apache.accumulo.core.client.AccumuloException;
+import org.apache.accumulo.core.client.AccumuloSecurityException;
+import org.apache.accumulo.core.client.ResourceGroupNotFoundException;
+import org.apache.accumulo.core.data.ResourceGroupId;
+
+/**
+ * A ResourceGroup is a grouping of Accumulo server processes that have some
shared characteristic
+ * that is different than server processes in other resource groups. Examples
could be homogeneous
+ * hardware configurations for the server processes in one resource group but
different than other
+ * resource groups, or a resource group could be created for physical
separation of processing for
+ * table(s).
+ *
+ * A default resource group exists in which all server processes are assigned.
The Manager, Monitor,
+ * and GarbageCollector are assigned to the default resource group. Compactor,
ScanServer and
+ * TabletServer processes are also assigned to the default resource group,
unless their respective
+ * properties are set.
+ *
+ * This object is for defining, interacting, and removing resource group
configurations. When the
+ * Accumulo server processes get the system configuration, they will receive a
merged view of the
+ * system configuration and applicable resource group configuration, with any
property defined in
+ * the resource group configuration given higher priority.
+ *
+ * @since 4.0.0
+ */
+public interface ResourceGroupOperations {
+
+ /**
+ * A method to check if a resource group configuration exists in Accumulo.
+ *
+ * @param group the name of the resource group
+ * @return true if the group exists
+ */
+ boolean exists(String group);
+
+ /**
+ * Retrieve a list of resource groups in Accumulo.
+ *
+ * @return Set of resource groups in accumulo
+ */
+ Set<ResourceGroupId> list();
+
+ /**
+ * Create a configuration node in zookeeper for a resource group. If not
defined, then processes
+ * running in the resource group will use the values of the properties
defined in the system
+ * configuration.
+ *
+ * @param group resource group
+ * @throws AccumuloException if a general error occurs
+ * @throws AccumuloSecurityException if the user does not have permission
+ */
+ void create(final ResourceGroupId group) throws AccumuloException,
AccumuloSecurityException;
+
+ /**
+ * Returns the properties set for this resource group in zookeeper merged
with the system
+ * configuration.
+ *
+ * @param group resource group
+ * @return Map of property keys/values
+ * @throws AccumuloException if a general error occurs
+ * @throws AccumuloSecurityException if the user does not have permission
+ * @throws ResourceGroupNotFoundException if the specified resource group
doesn't exist
+ */
+ Map<String,String> getConfiguration(final ResourceGroupId group)
+ throws AccumuloException, AccumuloSecurityException,
ResourceGroupNotFoundException;
+
+ /**
+ * Returns the un-merged properties set for this resource group in zookeeper.
+ *
+ * @param group resource group
+ * @return Map of property keys/values
+ * @throws AccumuloException if a general error occurs
+ * @throws AccumuloSecurityException if the user does not have permission
+ * @throws ResourceGroupNotFoundException if the specified resource group
doesn't exist
+ */
+ Map<String,String> getProperties(final ResourceGroupId group)
+ throws AccumuloException, AccumuloSecurityException,
ResourceGroupNotFoundException;
+
+ /**
+ * Sets a resource group property in zookeeper. Servers will pull this
setting and override the
+ * equivalent setting in accumulo.properties. Changes can be seen using
+ * {@code #getProperties(ResourceGroupId)} or {@link
InstanceOperations#getSystemConfiguration()}
+ * can be used to return a system-level merged view.
+ * <p>
+ * Only some properties can be changed by this method, an
IllegalArgumentException will be thrown
+ * if there is an attempt to set a read-only property.
+ *
+ * @param group resource group
+ * @param property the name of a system property
+ * @param value the value to set a system property to
+ * @throws AccumuloException if a general error occurs
+ * @throws AccumuloSecurityException if the user does not have permission
+ * @throws ResourceGroupNotFoundException if the specified resource group
doesn't exist
+ */
+ void setProperty(final ResourceGroupId group, final String property, final
String value)
+ throws AccumuloException, AccumuloSecurityException,
ResourceGroupNotFoundException;
+
+ /**
+ * Modify resource group properties using a Consumer that accepts a mutable
map containing the
+ * current system property overrides stored in ZooKeeper. If the supplied
Consumer alters the map
+ * without throwing an Exception, then the resulting map will atomically
replace the current
+ * system property overrides in ZooKeeper. Only properties which can be
stored in ZooKeeper will
+ * be accepted.
+ *
+ * <p>
+ * Accumulo has multiple layers of properties that for many APIs and SPIs
are presented as a
+ * single merged view. This API does not offer that merged view, it only
offers the properties set
+ * at the resource group layer to the mapMutator.
+ * </p>
+ *
+ * <p>
+ * Below is an example of using this API to conditionally set some instance
properties. If while
+ * trying to set the compaction planner properties another process modifies
the manager balancer
+ * properties, then it would automatically retry and call the lambda again
with the latest
+ * snapshot of instance properties.
+ * </p>
+ *
+ * <pre>
+ * {@code
+ * AccumuloClient client = getClient();
+ * Map<String,String> acceptedProps =
client.resourceGroupOperations().modifyProperties(groupId, currProps -> {
+ * var planner =
currProps.get("compaction.service.default.planner");
+ * //This code will only change the compaction planner if its
currently set to default settings.
+ * //The endsWith() function was used to make the example
short, would be better to use equals().
+ * if(planner != null &&
planner.endsWith("RatioBasedCompactionPlanner") {
+ * // tservers will eventually see these compaction planner
changes and when they do they will see all of the changes at once
+ * currProps.keySet().removeIf(
+ * prop ->
prop.startsWith("compaction.service.default.planner.opts."));
+ *
currProps.put("compaction.service.default.planner","MyPlannerClassName");
+ *
currProps.put("compaction.service.default.planner.opts.myOpt1","val1");
+ *
currProps.put("compaction.service.default.planner.opts.myOpt2","val2");
+ * }
+ * });
+ *
+ * // Since three properties were set may want to check for the
values of all
+ * // three, just checking one in this example to keep it short.
+ *
if("MyPlannerClassName".equals(acceptedProps.get("compaction.service.default.planner"))){
+ * // the compaction planner change was accepted or already
existed, so take action for that outcome
+ * } else {
+ * // the compaction planner change was not done, so take
action for that outcome
+ * }
+ * }
+ * }
+ * </pre>
+ *
+ * @param group resource group
+ * @param mapMutator This consumer should modify the passed snapshot of
resource group properties
+ * to contain the desired keys and values. It should be safe for
Accumulo to call this
+ * consumer multiple times, this may be done automatically when
certain retryable errors
+ * happen. The consumer should probably avoid accessing the Accumulo
client as that could
+ * lead to undefined behavior.
+ *
+ * @throws AccumuloException if a general error occurs
+ * @throws AccumuloSecurityException if the user does not have permission
+ * @throws IllegalArgumentException if the Consumer alters the map by adding
properties that
+ * cannot be stored in ZooKeeper
+ * @throws ResourceGroupNotFoundException if the specified resource group
doesn't exist
+ *
+ * @return The map that became Accumulo's new properties for this resource
group. This map is
+ * immutable and contains the snapshot passed to mapMutator and the
changes made by
+ * mapMutator.
+ */
+ Map<String,String> modifyProperties(final ResourceGroupId group,
+ final Consumer<Map<String,String>> mapMutator) throws AccumuloException,
+ AccumuloSecurityException, IllegalArgumentException,
ResourceGroupNotFoundException;
+
+ /**
+ * Removes a resource group property from zookeeper. Changes can be seen
using
+ * {@code #getProperties(ResourceGroupId)} or {@link
InstanceOperations#getSystemConfiguration()}
+ * can be used to return a system-level merged view.
+ *
+ * @param group resource group
+ * @param property the name of a system property
+ * @throws AccumuloException if a general error occurs
+ * @throws AccumuloSecurityException if the user does not have permission
+ * @throws ResourceGroupNotFoundException if the specified resource group
doesn't exist
+ */
+ void removeProperty(final ResourceGroupId group, final String property)
+ throws AccumuloException, AccumuloSecurityException,
ResourceGroupNotFoundException;
+
+ /**
+ * Removes a configuration node in zookeeper for a resource group. If not
defined, then processes
+ * running in the resource group will use the values of the properties
defined in the system
+ * configuration.
+ *
+ * @param group resource group
+ * @throws AccumuloException if a general error occurs
+ * @throws AccumuloSecurityException if the user does not have permission
+ * @throws ResourceGroupNotFoundException if the specified resource group
doesn't exist
+ */
+ void remove(final ResourceGroupId group)
Review Comment:
What happens if this is called w/ the default RG? Could document that in
javadoc.
--
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: notifications-unsubscr...@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
