ctubbsii commented on code in PR #5749: URL: https://github.com/apache/accumulo/pull/5749#discussion_r2308814478
########## core/src/main/java/org/apache/accumulo/core/clientImpl/ResourceGroupOperationsImpl.java: ########## @@ -0,0 +1,290 @@ +/* + * 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) { + checkArgument(group != null, "group argument must be supplied"); + 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))); + if (!groups.contains(ResourceGroupId.DEFAULT)) { + throw new IllegalStateException("Default resource group not found in ZooKeeper"); + } + return Set.copyOf(groups); + } + + @Override + public void create(ResourceGroupId group) throws AccumuloException, AccumuloSecurityException { + checkArgument(group != null, "group argument must be supplied"); + 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 { + checkArgument(group != null, "group argument must be supplied"); + Map<String,String> config = new HashMap<>(); + config.putAll(ThriftClientTypes.CLIENT.execute(context, client -> client + .getConfiguration(TraceUtil.traceInfo(), context.rpcCreds(), ConfigurationType.PROCESS))); + config.putAll(getProperties(group)); + return Map.copyOf(config); + } + + @Override + public Map<String,String> getProperties(ResourceGroupId group) + throws AccumuloException, AccumuloSecurityException, ResourceGroupNotFoundException { + checkArgument(group != null, "group argument must be supplied"); + try { + TVersionedProperties rgProps = ThriftClientTypes.CLIENT.execute(context, + client -> client.getVersionedResourceGroupProperties(TraceUtil.traceInfo(), + context.rpcCreds(), group.canonical())); + if (rgProps != null && rgProps.getPropertiesSize() > 0) { + return Map.copyOf(rgProps.getProperties()); + } else { + return Map.of(); + } + } catch (AccumuloException | AccumuloSecurityException e) { + Throwable t = e.getCause(); + if (t instanceof ThriftResourceGroupNotExistsException) { + ThriftResourceGroupNotExistsException te = (ThriftResourceGroupNotExistsException) t; Review Comment: Perfect use of `var`: ```suggestion var te = (ThriftResourceGroupNotExistsException) t; ``` ########## core/src/main/java/org/apache/accumulo/core/clientImpl/ResourceGroupOperationsImpl.java: ########## @@ -0,0 +1,290 @@ +/* + * 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) { + checkArgument(group != null, "group argument must be supplied"); Review Comment: I wish we hadn't started using IllegalArgumentException throughout our API for non-null checks. Nullness has its own methods for verification. I think our APIs probably use a mix of IAE and NPE, though. ########## server/base/src/main/java/org/apache/accumulo/server/ServerContext.java: ########## @@ -182,6 +191,10 @@ public SiteConfiguration getSiteConfiguration() { @Override public AccumuloConfiguration getConfiguration() { + return serverConfFactory.get().getResourceGroupConfiguration(); Review Comment: Is there ever a situation where this wouldn't apply? Or is everything using ServerContext using the default resource group? Also one note about the default resource group... it seems to be a convention, but not necessarily strictly protected (e.g. can you delete the default resource group?) ########## minicluster/src/main/java/org/apache/accumulo/cluster/ClusterControl.java: ########## @@ -49,12 +50,13 @@ public interface ClusterControl { /** * Starts all occurrences of the given server */ - void startAllServers(ServerType server) throws IOException; + void startAllServers(ServerType server) throws InterruptedException, IOException, KeeperException; /** * Start the given process on the host */ - void start(ServerType server, String hostname) throws IOException; + void start(ServerType server, String hostname) + throws InterruptedException, IOException, KeeperException; Review Comment: Do we need to propagate all the ZK exceptions? They seem to be leaking everywhere throughout our code, and I think we will probably need to do a better job of reining these in. ########## core/src/main/java/org/apache/accumulo/core/clientImpl/ResourceGroupOperationsImpl.java: ########## @@ -0,0 +1,290 @@ +/* + * 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) { + checkArgument(group != null, "group argument must be supplied"); + ResourceGroupId rg = ResourceGroupId.of(group); + return context.getZooCache().get(Constants.ZRESOURCEGROUPS + "/" + rg.canonical()) != null; + } + + @Override + public Set<ResourceGroupId> list() { Review Comment: It's a little weird to call a method called "list" that returns a "set". I like returning a set, though... not sure if there's a better name for the method. ########## core/src/main/java/org/apache/accumulo/core/data/ResourceGroupId.java: ########## @@ -18,15 +18,18 @@ */ package org.apache.accumulo.core.data; +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> { + public static final Pattern GROUP_NAME_PATTERN = Pattern.compile("^[a-zA-Z]+(_?[a-zA-Z0-9])*$"); Review Comment: This can be simplified substantially: ```suggestion public static final Pattern GROUP_NAME_PATTERN = Pattern.compile("^[a-zA-Z]\w*$"); ``` The main difference with this is that you can have repeated or trailing underscores, which I think is probably fine, if users really want such weird names. ########## 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: It's possible some RPC config may affect the ability to communicate, but other per-process settings could also cause that (like CLASSPATH). The "instance" properties are not expected to represent the exhaustive list of properties that must be set the same across the Accumulo instance... but the minimum set that must be the same. I wouldn't worry too much about the RPC ones. If you want to add them to the allowed set for resource groups, I think that's fine, but I don't care that much. We can start with a smaller set, and increase it as we receive feature requests. -- 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]
