keith-turner commented on code in PR #4851:
URL: https://github.com/apache/accumulo/pull/4851#discussion_r1742217403
##########
core/src/main/java/org/apache/accumulo/core/client/admin/InstanceOperations.java:
##########
@@ -181,41 +184,90 @@ Map<String,String>
modifyProperties(Consumer<Map<String,String>> mapMutator)
*
* @return a list of locations in <code>hostname:port</code> form.
* @since 2.1.0
+ * @deprecated see {@link #getServers(ServerTypeName)}
*/
+ @Deprecated(since = "4.0.0")
List<String> getManagerLocations();
/**
* Returns the locations of the active compactors
*
* @return A set of currently active compactors.
* @since 2.1.4
+ * @deprecated see {@link #getServers(ServerTypeName)}
*/
+ @Deprecated(since = "4.0.0")
Set<String> getCompactors();
/**
* Returns the locations of the active scan servers
*
* @return A set of currently active scan servers.
+ * @deprecated see {@link #getServers(ServerTypeName)}
*/
+ @Deprecated(since = "4.0.0")
Set<String> getScanServers();
/**
* List the currently active tablet servers participating in the accumulo
instance
*
* @return A list of currently active tablet servers.
+ * @deprecated see {@link #getServers(ServerTypeName)}
*/
+ @Deprecated(since = "4.0.0")
List<String> getTabletServers();
+ /**
+ * Resolve the server of the given type and address to a ServerId
+ *
+ * @param type type of server
+ * @param host host name
+ * @param port host port
+ * @return ServerId if found, else null
+ * @since 4.0.0
+ */
+ ServerId getServer(ServerTypeName type, String host, int port);
+
+ /**
+ * Returns all servers of the given types
+ *
+ * @return set of servers of the supplied types matching the supplied test
+ * @since 4.0.0
+ */
+ Set<ServerId> getServers(ServerTypeName type);
+
+ /**
+ * Returns the servers of a given type that match the given criteria
+ *
+ * @return set of servers of the supplied types matching the supplied test
+ * @since 4.0.0
+ */
+ Set<ServerId> getServers(ServerTypeName type, Predicate<ServerId> test);
Review Comment:
Posting this for consideration, not really recommending this change though
because I am uncertain about how it would actually play out.
It may be nice to limit what resource groups are fetched from ZK. If the ZK
schema supported this it could be really efficient at avoiding reading entire
RGs. The `Predicate<ServerId> test` can only filter after fetching everything
from ZK.
```java
Set<ServerId> getServers(ServerTypeName type, Predicate<String>
resourceGroupFilter)
```
May be a good follow on issue to try to figure how to do this pre fetch
filtering and do that at the same time as adding options to the shell for
listscans and liscompactions that allow filtering on resoruce groups and server
type. If we can figure out a good way to do this it may make those command
more responsive when inspecting a single RG or server type (like listscans on a
single scan server RG).
##########
core/src/main/java/org/apache/accumulo/core/client/admin/servers/ServerId.java:
##########
@@ -0,0 +1,107 @@
+/*
+ * 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.servers;
+
+import java.util.Objects;
+
+import org.apache.accumulo.core.conf.PropertyType.PortRange;
+
+import com.google.common.base.Preconditions;
+
+public abstract class ServerId implements Comparable<ServerId> {
Review Comment:
This should have javadoc w/ a since tag. Also the child types should have a
since tag.
##########
core/src/main/java/org/apache/accumulo/core/client/admin/servers/CompactorServerId.java:
##########
@@ -0,0 +1,27 @@
+/*
+ * 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.servers;
+
+public class CompactorServerId extends ServerId {
Review Comment:
If we can make the `ServerId` constructor package private and these child
classes final then that makes the types immutable. Currently the type could be
extended and made mutable.
```suggestion
public final class CompactorServerId extends ServerId {
```
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/ServerIds.java:
##########
@@ -0,0 +1,204 @@
+/*
+ * 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 java.util.concurrent.TimeUnit.MILLISECONDS;
+
+import java.util.HashMap;
+import java.util.HashSet;
+import java.util.List;
+import java.util.Map;
+import java.util.Optional;
+import java.util.Set;
+import java.util.UUID;
+
+import org.apache.accumulo.core.Constants;
+import org.apache.accumulo.core.client.admin.servers.CompactorServerId;
+import org.apache.accumulo.core.client.admin.servers.ManagerServerId;
+import org.apache.accumulo.core.client.admin.servers.ScanServerId;
+import org.apache.accumulo.core.client.admin.servers.TabletServerId;
+import org.apache.accumulo.core.fate.zookeeper.ZooCache;
+import org.apache.accumulo.core.fate.zookeeper.ZooCache.ZcStat;
+import org.apache.accumulo.core.lock.ServiceLock;
+import org.apache.accumulo.core.lock.ServiceLock.ServiceLockPath;
+import org.apache.accumulo.core.lock.ServiceLockData;
+import org.apache.accumulo.core.lock.ServiceLockData.ThriftService;
+import org.apache.accumulo.core.util.Timer;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+import com.google.common.net.HostAndPort;
+
+public class ServerIds {
Review Comment:
I didn't really look at this class, let me know if there is anything in it
you would like a 2nd pair of eyes on.
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/InstanceOperationsImpl.java:
##########
@@ -413,4 +447,56 @@ public InstanceId getInstanceId() {
return context.getInstanceID();
}
+ @Override
+ public ServerId getServer(ServerTypeName type, String host, int port) {
+ final HostAndPort hp = HostAndPort.fromParts(host, port);
+ switch (type) {
+ case COMPACTOR:
+ return context.getServerIdResolver().resolveCompactor(hp.toString());
+ case MANAGER:
+ final ManagerServerId msi = context.getServerIdResolver().getManager();
+ if (msi.getHost().equals(host) && msi.getPort() == port) {
+ return msi;
+ } else {
+ return null;
+ }
+ case SCAN_SERVER:
+ return context.getServerIdResolver().resolveScanServer(hp.toString());
+ case TABLET_SERVER:
+ return
context.getServerIdResolver().resolveTabletServer(hp.toString());
+ default:
+ throw new IllegalArgumentException("Unhandled server type: " + type);
+ }
+ }
+
+ @Override
+ public Set<ServerId> getServers(ServerTypeName type) {
+ return getServers(type, null);
+ }
+
+ @Override
+ public Set<ServerId> getServers(ServerTypeName type, Predicate<ServerId>
test) {
+ final Set<ServerId> results = new HashSet<>();
+ switch (type) {
+ case COMPACTOR:
+ results.addAll(context.getServerIdResolver().getCompactors());
+ break;
+ case MANAGER:
+ results.add(context.getServerIdResolver().getManager());
+ break;
+ case SCAN_SERVER:
+
results.addAll(context.getServerIdResolver().getScanServers().keySet());
+ break;
+ case TABLET_SERVER:
+ results.addAll(context.getServerIdResolver().getTabletServers());
+ break;
+ default:
+ break;
+ }
+ if (test == null) {
+ return results;
Review Comment:
Could avoid returning mutable sets.
```suggestion
return Collections.unmodifiableSet(results);
```
For the stream I think Collectors has a function for collecting to an
unmodifiable set.
##########
core/src/main/java/org/apache/accumulo/core/client/admin/ActiveCompaction.java:
##########
@@ -145,6 +145,8 @@ enum Type {
String getAddress();
int getPort();
+
+ String getResourceGroup();
Review Comment:
We should open a follow on issue to look into deprecating this
CompactionHost interface in favor of the new ServerId type.
##########
core/src/main/java/org/apache/accumulo/core/client/admin/InstanceOperations.java:
##########
@@ -225,10 +277,25 @@ List<ActiveScan> getActiveScans(String tserver)
* @param tserver The server address. This should be of the form {@code <ip
address>:<port>}
* @return the list of active compactions
* @since 1.5.0
+ * @deprecated see {@link #getActiveCompactions(ServerId server)}
*/
+ @Deprecated(since = "4.0.0")
List<ActiveCompaction> getActiveCompactions(String tserver)
throws AccumuloException, AccumuloSecurityException;
+ /**
+ * List the active compaction running on a TabletServer or Compactor. The
server address can be
+ * retrieved using {@link #getCompactors()} or {@link #getTabletServers()}.
Use
+ * {@link #getActiveCompactions()} to get a list of all compactions running
on tservers and
+ * compactors.
+ *
+ * @param server The ServerId object
+ * @return the list of active compactions
+ * @since 4.0.0
Review Comment:
This could have throws javadoc like getActiveScans does. Could also test
passing in unexpected server types in an IT.
##########
core/src/main/java/org/apache/accumulo/core/client/admin/InstanceOperations.java:
##########
@@ -181,41 +184,90 @@ Map<String,String>
modifyProperties(Consumer<Map<String,String>> mapMutator)
*
* @return a list of locations in <code>hostname:port</code> form.
* @since 2.1.0
+ * @deprecated see {@link #getServers(ServerTypeName)}
*/
+ @Deprecated(since = "4.0.0")
List<String> getManagerLocations();
/**
* Returns the locations of the active compactors
*
* @return A set of currently active compactors.
* @since 2.1.4
+ * @deprecated see {@link #getServers(ServerTypeName)}
*/
+ @Deprecated(since = "4.0.0")
Set<String> getCompactors();
/**
* Returns the locations of the active scan servers
*
* @return A set of currently active scan servers.
+ * @deprecated see {@link #getServers(ServerTypeName)}
*/
+ @Deprecated(since = "4.0.0")
Set<String> getScanServers();
/**
* List the currently active tablet servers participating in the accumulo
instance
*
* @return A list of currently active tablet servers.
+ * @deprecated see {@link #getServers(ServerTypeName)}
*/
+ @Deprecated(since = "4.0.0")
List<String> getTabletServers();
+ /**
+ * Resolve the server of the given type and address to a ServerId
+ *
+ * @param type type of server
+ * @param host host name
+ * @param port host port
+ * @return ServerId if found, else null
+ * @since 4.0.0
+ */
+ ServerId getServer(ServerTypeName type, String host, int port);
+
+ /**
+ * Returns all servers of the given types
+ *
+ * @return set of servers of the supplied types matching the supplied test
+ * @since 4.0.0
+ */
+ Set<ServerId> getServers(ServerTypeName type);
+
+ /**
+ * Returns the servers of a given type that match the given criteria
+ *
+ * @return set of servers of the supplied types matching the supplied test
+ * @since 4.0.0
+ */
+ Set<ServerId> getServers(ServerTypeName type, Predicate<ServerId> test);
+
/**
* List the active scans on a tablet server.
*
* @param tserver The tablet server address. This should be of the form
* {@code <ip address>:<port>}
* @return A list of active scans on tablet server.
+ * @deprecated see {@link #getActiveScans(ServerId)}
*/
+ @Deprecated(since = "4.0.0")
List<ActiveScan> getActiveScans(String tserver)
throws AccumuloException, AccumuloSecurityException;
+ /**
+ * List the active scans on a server.
+ *
+ * @param server server type and address
+ * @return A stream of active scans on server.
+ * @since 4.0.0
+ * @throws IllegalArgumentException when the type of the server is not
TABLET_SERVER or
Review Comment:
Would be nice to test passing unexpected server types to this method in an
IT.
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/InstanceOperationsImpl.java:
##########
@@ -279,6 +277,40 @@ public List<ActiveScan> getActiveScans(String tserver)
}
}
+ @Override
+ public List<ActiveScan> getActiveScans(ServerId server)
+ throws AccumuloException, AccumuloSecurityException {
+
+ @SuppressWarnings("unused")
+ var unused = Objects.nonNull(server);
+ Preconditions.checkArgument(server.getType() == ServerTypeName.SCAN_SERVER
+ || server.getType() == ServerTypeName.TABLET_SERVER);
Review Comment:
Not sure the Preconditions syntax is correct, attempted to add an error
message.
```suggestion
Preconditions.checkArgument(server.getType() ==
ServerTypeName.SCAN_SERVER
|| server.getType() == ServerTypeName.TABLET_SERVER, "Server type %s
is not %s or %s.", server.getType(), ServerTypeName.SCAN_SERVER,
ServerTypeName.TABLET_SERVER);
```
##########
core/src/main/java/org/apache/accumulo/core/client/admin/servers/ServerTypeName.java:
##########
@@ -0,0 +1,23 @@
+/*
+ * 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.servers;
+
+public enum ServerTypeName {
Review Comment:
Needs a since tag.
##########
core/src/main/java/org/apache/accumulo/core/client/admin/servers/ServerId.java:
##########
@@ -0,0 +1,107 @@
+/*
+ * 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.servers;
+
+import java.util.Objects;
+
+import org.apache.accumulo.core.conf.PropertyType.PortRange;
+
+import com.google.common.base.Preconditions;
+
+public abstract class ServerId implements Comparable<ServerId> {
+
+ private final ServerTypeName type;
+ private final String resourceGroup;
+ private final String host;
+ private final int port;
+
+ protected ServerId(ServerTypeName type, String resourceGroup, String host,
int port) {
Review Comment:
This suggestion of making the constructor package private goes w/ making the
child classes final.
```suggestion
ServerId(ServerTypeName type, String resourceGroup, String host, int port)
{
```
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/InstanceOperationsImpl.java:
##########
@@ -287,19 +319,33 @@ public boolean testClassLoad(final String className,
final String asTypeName)
}
@Override
+ @Deprecated
public List<ActiveCompaction> getActiveCompactions(String server)
throws AccumuloException, AccumuloSecurityException {
- final var serverHostAndPort = HostAndPort.fromString(server);
+ ServerId si = context.getServerIdResolver().resolveCompactor(server);
+ if (si == null) {
+ si = context.getServerIdResolver().resolveTabletServer(server);
+ }
+ if (si == null) {
+ return new ArrayList<>();
+ }
+ return getActiveCompactions(si);
+ }
+
+ @Override
+ public List<ActiveCompaction> getActiveCompactions(ServerId server)
+ throws AccumuloException, AccumuloSecurityException {
+
Review Comment:
This could have a Preconditions check to check the server type like
getActiveScans does.
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/InstanceOperationsImpl.java:
##########
@@ -413,4 +447,56 @@ public InstanceId getInstanceId() {
return context.getInstanceID();
}
+ @Override
+ public ServerId getServer(ServerTypeName type, String host, int port) {
+ final HostAndPort hp = HostAndPort.fromParts(host, port);
+ switch (type) {
+ case COMPACTOR:
+ return context.getServerIdResolver().resolveCompactor(hp.toString());
+ case MANAGER:
+ final ManagerServerId msi = context.getServerIdResolver().getManager();
+ if (msi.getHost().equals(host) && msi.getPort() == port) {
+ return msi;
+ } else {
+ return null;
+ }
+ case SCAN_SERVER:
+ return context.getServerIdResolver().resolveScanServer(hp.toString());
+ case TABLET_SERVER:
+ return
context.getServerIdResolver().resolveTabletServer(hp.toString());
+ default:
+ throw new IllegalArgumentException("Unhandled server type: " + type);
+ }
+ }
+
+ @Override
+ public Set<ServerId> getServers(ServerTypeName type) {
+ return getServers(type, null);
+ }
+
+ @Override
+ public Set<ServerId> getServers(ServerTypeName type, Predicate<ServerId>
test) {
+ final Set<ServerId> results = new HashSet<>();
+ switch (type) {
+ case COMPACTOR:
+ results.addAll(context.getServerIdResolver().getCompactors());
Review Comment:
Not sure this is worthwhile, wondering if would be useful to push the
predicate down. Please ignore this comment if it does not make sense.
```suggestion
results.addAll(context.getServerIdResolver(test).getCompactors());
```
--
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]
