keith-turner commented on code in PR #5749: URL: https://github.com/apache/accumulo/pull/5749#discussion_r2248293234
########## core/src/main/java/org/apache/accumulo/core/clientImpl/ResourceGroupOperationsImpl.java: ########## @@ -0,0 +1,313 @@ +/* + * 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.io.UncheckedIOException; +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.TServerClient; +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.apache.thrift.TException; +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<>(); + final String old = System.getProperty(TServerClient.DEBUG_RG); + + try { + System.setProperty(TServerClient.DEBUG_RG, group.canonical()); Review Comment: If multiple threads call this getConfiguration method then its behavior could be non-deterministic. Like another thread could set the java system prop to another RG after this thread set it. This could lead to getting info for the wrong RG. Passing the preferred RG as a parameter to the CLIENT.execute function would be more predictable. Also if this prop was set by the user when they started the java process, then silently ignoring it here seems confusing. If I am trying to debug and setting a very specfic prop to help me debug then I would not want it to be ignored, would rather have a failure so I know what is happening and can adjust. ########## core/src/main/java/org/apache/accumulo/core/rpc/clients/TServerClient.java: ########## @@ -60,16 +63,23 @@ public interface TServerClient<C extends TServiceClient> { static final String DEBUG_HOST = "org.apache.accumulo.client.rpc.debug.host"; + static final String DEBUG_RG = "org.apache.accumulo.client.rpc.debug.rg"; Review Comment: If both of these are set, should the code just fail? If not maybe we need to look and see if they are disjoint, like both match a non-empty server set on their own but their intersection yields empty an empty set. Might want to raise an error in this case informing that the two configs conflict w/ each other. ########## 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: Looked at those changes and saw some problems. However, the goal of trying to contact a server in the RG for seems good to me for now. Just need to fix this issues. We can always start off w/ more strict behavior and relax it later. > I'm thinking that a good solution for the config command would be to connect to a server in the RG and if there are none, then tell the user to use the zoo-info-viewer command. Is making it behave this way based on the assumption that its more likely that severs in the same RG will have the same site config? If so that seems reasonable to me. One thing I like about this PR is how its restrictive in what props it allows to be set at the RG level. In general wondering if we could eventually make similar changes to existing site and system config. Like if site config could only contain instance props and system config had the same restrictions as RG props, then it would remove some long standing potential problems. Also that could remove the need to contact a server in a RG group to get info that is stored in ZK and ideally any server could be contacted. ########## core/src/main/java/org/apache/accumulo/core/clientImpl/ResourceGroupOperationsImpl.java: ########## @@ -0,0 +1,313 @@ +/* + * 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.io.UncheckedIOException; +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.TServerClient; +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.apache.thrift.TException; +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<>(); + final String old = System.getProperty(TServerClient.DEBUG_RG); + + try { + System.setProperty(TServerClient.DEBUG_RG, group.canonical()); + config.putAll(ThriftClientTypes.CLIENT.execute(context, client -> client + .getConfiguration(TraceUtil.traceInfo(), context.rpcCreds(), ConfigurationType.PROCESS))); + } catch (AccumuloException e) { + if (e.getCause() instanceof TException + && e.getCause().getCause() instanceof ResourceGroupNotFoundException) { + throw new ResourceGroupNotFoundException(group.canonical()); + } + } catch (UncheckedIOException e) { + throw new AccumuloException( + "No running servers in resource group: " + group + + ". Start a server or use zoo-info-viewer to see the resource group configuration.", + e); + } finally { + if (old == null) { Review Comment: Restoring the original value of this java system prop is also tricky for the case multiple threads. -- 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
