ctubbsii commented on code in PR #3189:
URL: https://github.com/apache/accumulo/pull/3189#discussion_r1099104174
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/ClientContext.java:
##########
@@ -512,24 +513,28 @@ public List<String> getManagerLocations() {
OpTimer timer = null;
if (log.isTraceEnabled()) {
- log.trace("tid={} Looking up manager location in zookeeper.",
Thread.currentThread().getId());
+ log.trace("tid={} Looking up manager location in zookeeper at {}.",
+ Thread.currentThread().getId(), zLockManagerPath);
timer = new OpTimer().start();
}
- byte[] loc = zooCache.getLockData(zLockManagerPath);
+ ServiceLockData sld = zooCache.getLockData(zLockManagerPath);
+ String location = null;
+ if (sld != null) {
+ location = sld.getAddressString(ThriftService.MANAGER);
+ }
Review Comment:
You could make it so `sld` is never `null`, which would clean up some of
these `null` checks, which seems to be a lot of boilerplate. You could return
`Optional<ServiceLockData>` or a dummy `ServiceLockData` that represents the
"not found in ZK" case (like `ServiceLockData.MISSING` that has a `null`
address string).
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/ClientContext.java:
##########
@@ -402,11 +404,10 @@ public Map<String,Pair<UUID,String>> getScanServers() {
try {
final var zLockPath = ServiceLock.path(root + "/" + addr);
ZcStat stat = new ZcStat();
- byte[] lockData = ServiceLock.getLockData(getZooCache(), zLockPath,
stat);
- if (lockData != null) {
- String[] fields = new String(lockData, UTF_8).split(",", 2);
- UUID uuid = UUID.fromString(fields[0]);
- String group = fields[1];
+ ServiceLockData sld = ServiceLock.getLockData(getZooCache(),
zLockPath, stat);
Review Comment:
I like the idea of creating the ServiceLockData to abstract away some of
this implementation with more expressive APIs. I think that could be done
independently of adding the group concept. Doing that first may make it easier
to see just the changes required to add a tserver group feature, separately
from the code organization work.
##########
core/src/main/java/org/apache/accumulo/core/util/ServiceLockData.java:
##########
@@ -0,0 +1,223 @@
+/*
+ * 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.util;
+
+import java.util.Collections;
+import java.util.EnumMap;
+import java.util.HashSet;
+import java.util.Optional;
+import java.util.Set;
+import java.util.UUID;
+
+import com.google.common.net.HostAndPort;
+import com.google.gson.Gson;
+
+public class ServiceLockData implements Comparable<ServiceLockData> {
+
+ private static final Gson gson = new Gson();
+
+ /**
+ * Thrift Service list
+ */
+ public static enum ThriftService {
+ CLIENT,
+ COORDINATOR,
+ COMPACTOR,
+ FATE,
+ GC,
+ MANAGER,
+ NONE,
+ TABLET_INGEST,
+ TABLET_MANAGEMENT,
+ TABLET_SCAN,
+ TSERV
+ }
+
+ /**
+ * An object that describes a process, the group assigned to that process,
the Thrift service and
+ * the address to use to communicate with that service.
+ */
+ public static class ServiceDescriptor {
+
+ /**
+ * The group name that will be used when one is not specified.
+ */
+ public static final String DEFAULT_GROUP_NAME = "default";
+
+ private final UUID uuid;
+ private final ThriftService service;
+ private final String address;
+ private final String group;
+
+ public ServiceDescriptor(UUID uuid, ThriftService service, String address,
String group) {
+ super();
Review Comment:
Pretty sure you don't need to explicitly call the super constructor here...
especially since the super-class is just Object.
```suggestion
```
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/ClientContext.java:
##########
@@ -512,24 +513,28 @@ public List<String> getManagerLocations() {
OpTimer timer = null;
if (log.isTraceEnabled()) {
- log.trace("tid={} Looking up manager location in zookeeper.",
Thread.currentThread().getId());
+ log.trace("tid={} Looking up manager location in zookeeper at {}.",
+ Thread.currentThread().getId(), zLockManagerPath);
Review Comment:
Some of these log message changes seem unrelated to this PR. Might be useful
to do those separately, first, so they don't detract from reviews on the main
changes intended in this PR.
##########
core/src/main/java/org/apache/accumulo/core/util/ServiceLockData.java:
##########
@@ -0,0 +1,223 @@
+/*
+ * 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.util;
+
+import java.util.Collections;
+import java.util.EnumMap;
+import java.util.HashSet;
+import java.util.Optional;
+import java.util.Set;
+import java.util.UUID;
+
+import com.google.common.net.HostAndPort;
+import com.google.gson.Gson;
+
+public class ServiceLockData implements Comparable<ServiceLockData> {
+
+ private static final Gson gson = new Gson();
+
+ /**
+ * Thrift Service list
+ */
+ public static enum ThriftService {
+ CLIENT,
+ COORDINATOR,
+ COMPACTOR,
+ FATE,
+ GC,
+ MANAGER,
+ NONE,
+ TABLET_INGEST,
+ TABLET_MANAGEMENT,
+ TABLET_SCAN,
+ TSERV
+ }
+
+ /**
+ * An object that describes a process, the group assigned to that process,
the Thrift service and
+ * the address to use to communicate with that service.
+ */
+ public static class ServiceDescriptor {
+
+ /**
+ * The group name that will be used when one is not specified.
+ */
+ public static final String DEFAULT_GROUP_NAME = "default";
+
+ private final UUID uuid;
+ private final ThriftService service;
+ private final String address;
+ private final String group;
+
+ public ServiceDescriptor(UUID uuid, ThriftService service, String address,
String group) {
+ super();
+ this.uuid = uuid;
+ this.service = service;
+ this.address = address;
+ this.group = Optional.ofNullable(group).orElse(DEFAULT_GROUP_NAME);
+ }
+
+ public UUID getUUID() {
+ return uuid;
+ }
+
+ public ThriftService getService() {
+ return service;
+ }
+
+ public String getAddress() {
+ return address;
+ }
+
+ public String getGroup() {
+ return group;
+ }
+
+ @Override
+ public int hashCode() {
+ return toString().hashCode();
+ }
+
+ @Override
+ public boolean equals(Object o) {
+ if (o instanceof ServiceDescriptor) {
Review Comment:
I think `instanceof` has gotten us in trouble before with `equals`. It will
return true for subclasses, but the subclass may not have a reciprocal equals
method implemented. Might want to instead check class same-ness rather than
`instanceof`.
##########
core/src/main/java/org/apache/accumulo/core/util/ServiceLockData.java:
##########
@@ -0,0 +1,223 @@
+/*
+ * 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.util;
+
+import java.util.Collections;
+import java.util.EnumMap;
+import java.util.HashSet;
+import java.util.Optional;
+import java.util.Set;
+import java.util.UUID;
+
+import com.google.common.net.HostAndPort;
+import com.google.gson.Gson;
+
+public class ServiceLockData implements Comparable<ServiceLockData> {
+
+ private static final Gson gson = new Gson();
+
+ /**
+ * Thrift Service list
+ */
+ public static enum ThriftService {
+ CLIENT,
+ COORDINATOR,
+ COMPACTOR,
+ FATE,
+ GC,
+ MANAGER,
+ NONE,
+ TABLET_INGEST,
+ TABLET_MANAGEMENT,
+ TABLET_SCAN,
+ TSERV
+ }
+
+ /**
+ * An object that describes a process, the group assigned to that process,
the Thrift service and
+ * the address to use to communicate with that service.
+ */
+ public static class ServiceDescriptor {
+
+ /**
+ * The group name that will be used when one is not specified.
+ */
+ public static final String DEFAULT_GROUP_NAME = "default";
+
+ private final UUID uuid;
+ private final ThriftService service;
+ private final String address;
+ private final String group;
+
+ public ServiceDescriptor(UUID uuid, ThriftService service, String address,
String group) {
+ super();
+ this.uuid = uuid;
+ this.service = service;
+ this.address = address;
+ this.group = Optional.ofNullable(group).orElse(DEFAULT_GROUP_NAME);
+ }
+
+ public UUID getUUID() {
+ return uuid;
+ }
+
+ public ThriftService getService() {
+ return service;
+ }
+
+ public String getAddress() {
+ return address;
+ }
+
+ public String getGroup() {
+ return group;
+ }
+
+ @Override
+ public int hashCode() {
+ return toString().hashCode();
+ }
+
+ @Override
+ public boolean equals(Object o) {
+ if (o instanceof ServiceDescriptor) {
+ return toString().equals(o.toString());
+ }
+ return false;
+ }
+
+ @Override
+ public String toString() {
+ return gson.toJson(this);
+ }
+
+ }
+
+ /**
+ * A set of ServiceDescriptor's
+ */
+ public static class ServiceDescriptors {
+ private final Set<ServiceDescriptor> descriptors;
+
+ public ServiceDescriptors() {
+ descriptors = new HashSet<>();
+ }
+
+ public ServiceDescriptors(HashSet<ServiceDescriptor> descriptors) {
+ this.descriptors = descriptors;
+ }
+
+ public void addService(ServiceDescriptor sd) {
+ this.descriptors.add(sd);
+ }
+
+ public Set<ServiceDescriptor> getServices() {
+ return descriptors;
+ }
+
+ @Override
+ public String toString() {
+ return gson.toJson(this);
+ }
+
+ public static ServiceDescriptors parse(String data) {
+ return gson.fromJson(data, ServiceDescriptors.class);
+ }
+ }
+
+ private EnumMap<ThriftService,ServiceDescriptor> services;
+
+ public ServiceLockData(ServiceDescriptors sds) {
+ this.services = new EnumMap<>(ThriftService.class);
+ sds.getServices().forEach(sd -> {
+ this.services.put(sd.getService(), sd);
+ });
Review Comment:
```suggestion
sds.getServices().forEach(sd -> this.services.put(sd.getService(), sd));
```
##########
minicluster/src/test/resources/log4j2-test.properties:
##########
@@ -33,6 +33,6 @@ logger.01.level = error
logger.02.name = org.apache.zookeeper
logger.02.level = error
-rootLogger.level = info
+rootLogger.level = debug
Review Comment:
Is this temporary? I hope this doesn't make our tests during the build too
spammy.
##########
server/base/src/main/java/org/apache/accumulo/server/ServerOpts.java:
##########
@@ -27,6 +28,17 @@ public class ServerOpts extends ConfigOpts {
@Parameter(names = {"-a", "--address"}, description = "address to bind to")
private String address = null;
+ @Parameter(required = false, names = {"-g", "--group"},
+ description = "Optional group name that will be made available to the
client (e.g. ScanServerSelector) "
+ + "and server (e.g. Balancers) plugins. If not specified will be
set to '"
+ + ServiceLockData.ServiceDescriptor.DEFAULT_GROUP_NAME
+ + "'. Assigning servers to groups support dedicating server
resources for specific purposes, where supported.")
+ private String groupName =
ServiceLockData.ServiceDescriptor.DEFAULT_GROUP_NAME;
Review Comment:
I've never been a fan of these command-line options separate from our config
file settings. The `-a` was added as a fix, but the semantics of it have always
been confusing, as in... is it the "bind address" or is it the "service
advertisement" address? It has always been conflated as both, with a special
case for `0.0.0.0` and `localhost`, but it should always have been two.
I'm mentioning this now, because I don't think we should continue the
precedent of adding new command-line options. Instead, there should be a
`tserver.group` attribute that can be put in the config file or specified on
the command-line, like any other config file option can be, with `-o
tserver.group=X`. (Similarly, we should have `tserver.address.listen` and
`tserver.address.advertise` as separate properties that default to `0.0.0.0`
and `AUTO` by default, but that's a separate issue. My main point here is that
we shouldn't continue the previous precedent... we should do it via config).
##########
core/src/main/java/org/apache/accumulo/core/util/ServiceLockData.java:
##########
@@ -0,0 +1,223 @@
+/*
+ * 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.util;
+
+import java.util.Collections;
+import java.util.EnumMap;
+import java.util.HashSet;
+import java.util.Optional;
+import java.util.Set;
+import java.util.UUID;
+
+import com.google.common.net.HostAndPort;
+import com.google.gson.Gson;
+
+public class ServiceLockData implements Comparable<ServiceLockData> {
+
+ private static final Gson gson = new Gson();
+
+ /**
+ * Thrift Service list
+ */
+ public static enum ThriftService {
+ CLIENT,
+ COORDINATOR,
+ COMPACTOR,
+ FATE,
+ GC,
+ MANAGER,
+ NONE,
+ TABLET_INGEST,
+ TABLET_MANAGEMENT,
+ TABLET_SCAN,
+ TSERV
+ }
+
+ /**
+ * An object that describes a process, the group assigned to that process,
the Thrift service and
+ * the address to use to communicate with that service.
+ */
+ public static class ServiceDescriptor {
+
+ /**
+ * The group name that will be used when one is not specified.
+ */
+ public static final String DEFAULT_GROUP_NAME = "default";
+
+ private final UUID uuid;
+ private final ThriftService service;
+ private final String address;
+ private final String group;
+
+ public ServiceDescriptor(UUID uuid, ThriftService service, String address,
String group) {
+ super();
+ this.uuid = uuid;
+ this.service = service;
+ this.address = address;
+ this.group = Optional.ofNullable(group).orElse(DEFAULT_GROUP_NAME);
+ }
+
+ public UUID getUUID() {
+ return uuid;
+ }
+
+ public ThriftService getService() {
+ return service;
+ }
+
+ public String getAddress() {
+ return address;
+ }
+
+ public String getGroup() {
+ return group;
+ }
+
+ @Override
+ public int hashCode() {
+ return toString().hashCode();
+ }
+
+ @Override
+ public boolean equals(Object o) {
+ if (o instanceof ServiceDescriptor) {
+ return toString().equals(o.toString());
+ }
+ return false;
+ }
+
+ @Override
+ public String toString() {
+ return gson.toJson(this);
+ }
+
+ }
+
+ /**
+ * A set of ServiceDescriptor's
+ */
+ public static class ServiceDescriptors {
+ private final Set<ServiceDescriptor> descriptors;
+
+ public ServiceDescriptors() {
+ descriptors = new HashSet<>();
+ }
+
+ public ServiceDescriptors(HashSet<ServiceDescriptor> descriptors) {
+ this.descriptors = descriptors;
+ }
+
+ public void addService(ServiceDescriptor sd) {
+ this.descriptors.add(sd);
+ }
+
+ public Set<ServiceDescriptor> getServices() {
+ return descriptors;
+ }
+
+ @Override
+ public String toString() {
+ return gson.toJson(this);
+ }
+
+ public static ServiceDescriptors parse(String data) {
+ return gson.fromJson(data, ServiceDescriptors.class);
+ }
+ }
+
+ private EnumMap<ThriftService,ServiceDescriptor> services;
+
+ public ServiceLockData(ServiceDescriptors sds) {
+ this.services = new EnumMap<>(ThriftService.class);
+ sds.getServices().forEach(sd -> {
+ this.services.put(sd.getService(), sd);
+ });
+ }
+
+ public ServiceLockData(UUID uuid, String address, ThriftService service,
String group) {
+ this(new ServiceDescriptors(new HashSet<>(
+ Collections.singleton(new ServiceDescriptor(uuid, service, address,
group)))));
Review Comment:
```suggestion
this(new ServiceDescriptors(Set.of(new ServiceDescriptor(uuid, service,
address, group))));
```
--
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]
