keith-turner commented on code in PR #5749: URL: https://github.com/apache/accumulo/pull/5749#discussion_r2231461857
########## core/src/main/java/org/apache/accumulo/core/client/admin/ResourceGroupOperations.java: ########## @@ -0,0 +1,232 @@ +/* + * 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 + * @since 4.0.0 Review Comment: Do not need since tags for methods when the version is the same as the class level since tag. ```suggestion ``` ########## core/src/main/java/org/apache/accumulo/core/client/admin/ResourceGroupOperations.java: ########## @@ -0,0 +1,232 @@ +/* + * 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 Review Comment: Another important use case that could be explicitly mentioned is dedicating resources for scan, ingest,compaction. ########## server/base/src/main/java/org/apache/accumulo/server/conf/util/ZooInfoViewer.java: ########## @@ -169,6 +174,7 @@ private void printProps(final ServerContext context, final Opts opts, final Prin } else { log.info("Filters:"); log.info("system: {}", opts.printSysProps()); + log.info("resource groups: {}", opts.printResourceGroupProps()); Review Comment: For namespaces and tables this will list them, seems like this should do the same for consistency. ########## server/base/src/main/java/org/apache/accumulo/server/conf/util/ZooInfoViewer.java: ########## @@ -460,6 +516,10 @@ boolean printSysProps() { return printAllProps() || printSystemOpt; } + boolean printResourceGroupProps() { + return printAllProps() || resourceGroupOpt.isEmpty(); Review Comment: These two changes would make rg more consistent w/ how tables and namespaces work in this code. ```java boolean printAllProps() { return !printSystemOpt && namespacesOpt.isEmpty() && tablesOpt.isEmpty() && resourceGroupOpt.isEmpty(); } ``` ```suggestion return printAllProps() || !resourceGroupOpt.isEmpty(); ``` ########## shell/src/main/java/org/apache/accumulo/shell/commands/ConfigCommand.java: ########## @@ -436,6 +480,8 @@ public Options getOptions() { outputFileOpt = new Option("o", "output", true, "local file to write the scan output to"); namespaceOpt = new Option(ShellOptions.namespaceOption, "namespace", true, "namespace to display/set/delete properties for"); + resourceGroupOpt = new Option(ShellOptions.resourceGroupOption, "resourceGroup", true, Review Comment: > The ConfigCommand displays all resource group values when listing properties Always displaying all seems useful, especially when filtering and grepping for stuff. Otherwise would need to loop over all RGs and filter/grep each. ########## server/base/src/main/java/org/apache/accumulo/server/init/ZooKeeperInitializer.java: ########## @@ -76,16 +77,33 @@ void initializeConfig(final InstanceId instanceId, final ZooReaderWriter zoo) { String zkInstanceRoot = ZooUtil.getRoot(instanceId); zoo.putPersistentData(zkInstanceRoot, EMPTY_BYTE_ARRAY, ZooUtil.NodeExistsPolicy.SKIP); var sysPropPath = SystemPropKey.of().getPath(); - VersionedProperties vProps = new VersionedProperties(); // skip if the encoded props node exists - if (zoo.exists(sysPropPath)) { + boolean alreadyExists = zoo.exists(zkInstanceRoot + sysPropPath); + var created = zoo.putPrivatePersistentData(zkInstanceRoot + sysPropPath, + VersionedPropCodec.getDefault().toBytes(new VersionedProperties()), + ZooUtil.NodeExistsPolicy.FAIL); + if (!alreadyExists && !created) { + throw new IllegalStateException( + "Failed to create default system props during initialization at: " + sysPropPath); + } + Review Comment: Could `ResourceGroupPropKey.createZNode(...)` be used here? ########## server/base/src/main/java/org/apache/accumulo/server/conf/ServerConfigurationFactory.java: ########## @@ -75,13 +77,14 @@ public class ServerConfigurationFactory extends ServerConfiguration { private final ConfigRefreshRunner refresher; - public ServerConfigurationFactory(ServerContext context, SiteConfiguration siteConfig) { + public ServerConfigurationFactory(ServerContext context, SiteConfiguration siteConfig, Review Comment: System configuration in accumulo is a named concept and silently merging the layers here seems confusing. I think we could make this more explict and it would not be a large change, but would allow more precise control over what configration we are actually getting. Maybe could do something like the following. ```java class ServerConfigurationFactory { // restore this method to its orginal behavior public AccumuloConfiguration getSystemConfiguration(); /** * Returns the merged view of configuration for the current resource group. */ public AccumuloConfiguration getResourceGroupConfiguration(); } // Then could make the following changes class ServerContext { // add this for any code that wants the system config.. need to look through the calls to // getConfiguration() and see what needs to switch to this. Suspect its not much. public AccumuloConfiguration getSystemConfiguration() { return serverConfFactory.get().getSystemConfiguration(); } public AccumuloConfiguration getConfiguration() { return serverConfFactory.get().getResourceGroupConfiguration(); } } // could restore ClientServiceHandler to return the system configuration when asked for it. // This would make the client api operation more predictable and preserve its current behavior. // This change may help w/ the shell behavior. class ClientServiceHandler { @Override public Map<String,String> getConfiguration(TInfo tinfo, TCredentials credentials, ConfigurationType type) throws TException { ... // one place that could use the new method on context return conf(credentials, context.getSystemConfiguration()); ... } } ``` ########## shell/src/main/java/org/apache/accumulo/shell/commands/ResourceGroupCommand.java: ########## @@ -0,0 +1,83 @@ +/* + * 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.shell.commands; + +import java.util.Set; +import java.util.TreeSet; + +import org.apache.accumulo.core.client.admin.ResourceGroupOperations; +import org.apache.accumulo.core.data.ResourceGroupId; +import org.apache.accumulo.shell.Shell; +import org.apache.accumulo.shell.Shell.Command; +import org.apache.commons.cli.CommandLine; +import org.apache.commons.cli.Option; +import org.apache.commons.cli.Options; + +public class ResourceGroupCommand extends Command { + + private Option createOpt; + private Option listOpt; + private Option deleteOpt; + + @Override + public int execute(String fullCommand, CommandLine cl, Shell shellState) throws Exception { + + final ResourceGroupOperations ops = shellState.getAccumuloClient().resourceGroupOperations(); + + if (cl.hasOption(createOpt.getOpt())) { + ops.create(ResourceGroupId.of(cl.getOptionValue(createOpt))); + } else if (cl.hasOption(deleteOpt.getOpt())) { + ops.remove(ResourceGroupId.of(cl.getOptionValue(deleteOpt))); + } else if (cl.hasOption(listOpt.getOpt())) { + Set<String> sorted = new TreeSet<>(); + ops.list().forEach(rg -> sorted.add(rg.canonical())); + shellState.printLines(sorted.iterator(), false); + } + return 0; + } + + @Override + public Options getOptions() { + final Options o = new Options(); + + createOpt = new Option("c", "create", true, "create a resource group"); + createOpt.setArgName("group"); + o.addOption(createOpt); + + listOpt = new Option("l", "list", false, "display resource group names"); + o.addOption(listOpt); + + deleteOpt = new Option("d", "delete", true, "delete a resource group"); + deleteOpt.setArgName("group"); + o.addOption(deleteOpt); + + return o; + } + + @Override + public String description() { + return "create, list, or remove resource groups"; Review Comment: > do we need separate createresourcegroup and deleteresourcegroup commands? That would follow the pattern of some other shell command. I like the pattern you have here though. Not sure if its possible in the shell, a nicer pattern would be subcommand could have their own options. Like`resourcegroup create -propsFile <initial props file> <rg name>` where only the create sub command has the -propFile option. If sub commands are not possible in the shell code then that leads to multiple top level commands for having per command options OR just having options that cause runtime failures when used together. ########## core/src/main/java/org/apache/accumulo/core/clientImpl/ResourceGroupOperationsImpl.java: ########## @@ -0,0 +1,277 @@ +/* + * 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) { + return list().contains(ResourceGroupId.of(group)); Review Comment: This could be made more efficient by checking if this exact group exist in ZK. This avoids scanning all groups which could be expensive if something ever calls this function inside a loop. Maybe something like the following. ```suggestion ResourceGroupId.of(group); // called for validation, should this method take a ResourceGroupId? return context.getZooCache().get(Constants.ZRESOURCEGROUPS+"/"+group) != null; ``' ########## core/src/main/java/org/apache/accumulo/core/client/admin/ResourceGroupOperations.java: ########## @@ -0,0 +1,232 @@ +/* + * 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 + * @since 4.0.0 + */ + boolean exists(String group); + + /** + * Retrieve a list of resource groups in Accumulo. + * + * @return Set of resource groups in accumulo + * @since 4.0.0 + */ + 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. Review Comment: >2. Modify accumulo-cluster to create the resource group configuration nodes. This could be done optionally, maybe something like the following. Then the accumulo cluster script will automatically create any missing resource groups. However this is not the default behavior, so an incorrect cluster.yaml file would fail instead of creating unwanted resource groups. ``` accumulo-cluster start --tservers --create-resource-groups INFO : Created resource group compactors-small with no resource group specific configuration. ``` When that option is not specified we could check each RG in accumulo-cluster and avoid starting any servers when it does not exist. ``` accumulo-cluster start --tservers WARN : The resouce group compactors-small does not exists, not starting any servers in it. ``` ########## server/base/src/main/java/org/apache/accumulo/server/util/ResourceGroupPropUtil.java: ########## @@ -0,0 +1,87 @@ +/* + * 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 static org.apache.accumulo.core.conf.Property.COMPACTION_PREFIX; +import static org.apache.accumulo.core.conf.Property.COMPACTOR_PREFIX; +import static org.apache.accumulo.core.conf.Property.SSERV_PREFIX; +import static org.apache.accumulo.core.conf.Property.TSERV_PREFIX; + +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.startsWith(COMPACTION_PREFIX.getKey()) + || property.startsWith(COMPACTOR_PREFIX.getKey()) + || property.startsWith(SSERV_PREFIX.getKey()) + || property.startsWith(TSERV_PREFIX.getKey())) { Review Comment: Putting it in Property seems good, would be co-located w/ lots of similar code. -- 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
