keith-turner commented on code in PR #4861:
URL: https://github.com/apache/accumulo/pull/4861#discussion_r1758999663
##########
server/base/src/main/java/org/apache/accumulo/server/manager/LiveTServerSet.java:
##########
@@ -227,16 +229,22 @@ public synchronized void scanServers() {
try {
final Set<TServerInstance> updates = new HashSet<>();
final Set<TServerInstance> doomed = new HashSet<>();
+ final ZooCache zc = getZooCache();
+ final String tserverRoot = context.getZooKeeperRoot() +
Constants.ZTSERVERS;
- final String path = context.getZooKeeperRoot() + Constants.ZTSERVERS;
+ Set<ServiceLockPath> tservers = new HashSet<>();
- HashSet<String> all = new HashSet<>(current.keySet());
- all.addAll(getZooCache().getChildren(path));
+ for (String resourceGroup : zc.getChildren(tserverRoot)) {
Review Comment:
If I looked at the code 3 months from now I may think this loop could be
replaced with a method call to the new ServiceLockPaths class. Could add a new
method like the following and call it here. That would make this nuance in the
code easier to understand and put the code in one place.
```java
class ServiceLockPaths {
/**
* @return all tservers lock paths in zookeeper, even those that do
not currently have any lock nodes beneath them
*/
Set<ServiceLockPath> getAllTserverLockPaths() {
}
}
```
Alternatively could add a comment to this loop mentioning why its not using
ServiceLockPaths.
##########
server/base/src/main/java/org/apache/accumulo/server/util/ServiceStatusCmd.java:
##########
@@ -99,91 +97,70 @@ StatusSummary getManagerStatus(final ZooReader zooReader,
String zRootPath) {
* lock data providing a service descriptor with host and port.
*/
@VisibleForTesting
- StatusSummary getMonitorStatus(final ZooReader zooReader, String zRootPath) {
- String lockPath = zRootPath + Constants.ZMONITOR_LOCK;
+ StatusSummary getMonitorStatus(final ZooReader zooReader, ServerContext
context) {
+ String lockPath = context.getServerPaths().createMonitorPath().toString();
return getStatusSummary(ServiceStatusReport.ReportKey.MONITOR, zooReader,
lockPath);
}
/**
- * The tserver paths in ZooKeeper are: {@code
/accumulo/[IID]/tservers/[host:port]/zlock#[NUM]}
- * with the lock data providing TSERV_CLIENT=host:port.
+ * The tserver paths in ZooKeeper are:
+ * {@code /accumulo/[IID]/tservers/[resourceGroup]/[host:port]/zlock#[NUM]}
with the lock data
+ * providing TSERV_CLIENT=host:port.
*/
@VisibleForTesting
- StatusSummary getTServerStatus(final ZooReader zooReader, String zRootPath) {
- String lockPath = zRootPath + Constants.ZTSERVERS;
- return getServerHostStatus(zooReader, lockPath,
ServiceStatusReport.ReportKey.T_SERVER, TSERV);
+ StatusSummary getTServerStatus(ServerContext context) {
+ final AtomicInteger errors = new AtomicInteger(0);
+ final Map<String,Set<String>> hostsByGroups = new TreeMap<>();
+ final Set<ServiceLockPath> compactors =
+ context.getServerPaths().getTabletServer(Optional.empty(),
Optional.empty());
+ compactors.forEach(c -> hostsByGroups
+ .computeIfAbsent(c.getResourceGroup(), (k) -> new
TreeSet<>()).add(c.getServer()));
+ return new StatusSummary(ServiceStatusReport.ReportKey.T_SERVER,
hostsByGroups.keySet(),
+ hostsByGroups, errors.get());
}
/**
- * The sserver paths in ZooKeeper are: {@code
/accumulo/[IID]/sservers/[host:port]/zlock#[NUM]}
- * with the lock data providing [UUID],[GROUP]
+ * The sserver paths in ZooKeeper are:
+ * {@code /accumulo/[IID]/sservers/[resourceGroup]/[host:port]/zlock#[NUM]}
with the lock data
+ * providing [UUID],[GROUP]
*/
@VisibleForTesting
- StatusSummary getScanServerStatus(final ZooReader zooReader, String
zRootPath) {
- String lockPath = zRootPath + Constants.ZSSERVERS;
- return getServerHostStatus(zooReader, lockPath,
ServiceStatusReport.ReportKey.S_SERVER,
- TABLET_SCAN);
- }
-
- /**
- * handles paths for tservers and servers with the lock stored beneath the
host: port like:
- * {@code /accumulo/IID/[tservers | sservers]/HOST:PORT/[LOCK]}
- */
- private StatusSummary getServerHostStatus(final ZooReader zooReader, String
basePath,
- ServiceStatusReport.ReportKey displayNames,
ServiceLockData.ThriftService serviceType) {
- AtomicInteger errorSum = new AtomicInteger(0);
-
- // Set<String> hostNames = new TreeSet<>();
- Set<String> groupNames = new TreeSet<>();
- Map<String,Set<String>> hostsByGroups = new TreeMap<>();
-
- var nodeNames = readNodeNames(zooReader, basePath);
-
- nodeNames.getData().forEach(host -> {
- var lock = readNodeNames(zooReader, basePath + "/" + host);
- lock.getData().forEach(l -> {
- var nodeData = readNodeData(zooReader, basePath + "/" + host + "/" +
l);
- int err = nodeData.getErrorCount();
- if (err > 0) {
- errorSum.addAndGet(nodeData.getErrorCount());
- } else {
-
- ServiceLockData.ServiceDescriptors sld =
- ServiceLockData.parseServiceDescriptors(nodeData.getData());
-
- sld.getServices().forEach(sd -> {
- if (serviceType == sd.getService()) {
- groupNames.add(sd.getGroup());
- hostsByGroups.computeIfAbsent(sd.getGroup(), set -> new
TreeSet<>())
- .add(sd.getAddress());
- }
- });
- }
- });
- errorSum.addAndGet(lock.getFirst());
- });
- return new StatusSummary(displayNames, groupNames, hostsByGroups,
errorSum.get());
+ StatusSummary getScanServerStatus(ServerContext context) {
+ final AtomicInteger errors = new AtomicInteger(0);
+ final Map<String,Set<String>> hostsByGroups = new TreeMap<>();
+ final Set<ServiceLockPath> scanServers =
+ context.getServerPaths().getScanServer(Optional.empty(),
Optional.empty());
+ scanServers.forEach(c -> hostsByGroups
+ .computeIfAbsent(c.getResourceGroup(), (k) -> new
TreeSet<>()).add(c.getServer()));
+ return new StatusSummary(ServiceStatusReport.ReportKey.S_SERVER,
hostsByGroups.keySet(),
+ hostsByGroups, errors.get());
}
/**
* The gc paths in ZooKeeper are: {@code
/accumulo/[IID]/gc/lock/zlock#[NUM]} with the lock data
* providing GC_CLIENT=host:port
*/
@VisibleForTesting
- StatusSummary getGcStatus(final ZooReader zooReader, String zRootPath) {
- String lockPath = zRootPath + Constants.ZGC_LOCK;
+ StatusSummary getGcStatus(final ZooReader zooReader, ServerContext context) {
+ String lockPath =
context.getServerPaths().createGarbageCollectorPath().toString();
return getStatusSummary(ServiceStatusReport.ReportKey.GC, zooReader,
lockPath);
}
/**
* The compactor paths in ZooKeeper are:
- * {@code /accumulo/[IID]/compactors/[QUEUE_NAME]/host:port/zlock#[NUM]}
with the host:port pulled
- * from the path
+ * {@code /accumulo/[IID]/compactors/[resourceGroup]/host:port/zlock#[NUM]}
with the host:port
Review Comment:
The ServiceLockPaths class is kinda abstracting how things are stored in ZK
from the rest of the code. Wondering if it should be removed or if it could be
modified and moved to ServiceLockPaths. Also some of the other methods in this
class have similar javadoc.
##########
server/manager/src/main/java/org/apache/accumulo/manager/Manager.java:
##########
@@ -640,26 +640,25 @@ private class ScanServerZKCleaner implements Runnable {
public void run() {
final ZooReaderWriter zrw = getContext().getZooReaderWriter();
- final String sserverZNodePath = getContext().getZooKeeperRoot() +
Constants.ZSSERVERS;
while (stillManager()) {
try {
- for (String sserverClientAddress :
zrw.getChildren(sserverZNodePath)) {
+ Set<ServiceLockPath> scanServerPaths =
+ getContext().getServerPaths().getScanServer(Optional.empty(),
Optional.empty());
+ for (ServiceLockPath path : scanServerPaths) {
- final String sServerZPath = sserverZNodePath + "/" +
sserverClientAddress;
- final var zLockPath = ServiceLock.path(sServerZPath);
ZcStat stat = new ZcStat();
Optional<ServiceLockData> lockData =
- ServiceLock.getLockData(getContext().getZooCache(), zLockPath,
stat);
+ ServiceLock.getLockData(getContext().getZooCache(), path,
stat);
if (lockData.isEmpty()) {
try {
- log.debug("Deleting empty ScanServer ZK node {}",
sServerZPath);
- zrw.delete(sServerZPath);
+ log.debug("Deleting empty ScanServer ZK node {}", path);
Review Comment:
This code was cleaning up empty scan server lock nodes, probably will not
work now. Wondering if we should have a third argument on methods like
`getContext().getServerPaths().getScanServer(` that specifies if the paths
must have locks beneath them or not. That would make each use consider if that
is important or not.
Not something for this PR, we probably need an IT that checks that empty
server nodes eventually go away in ZK. I will open an issue about that. Seems
like it would be good to ensure that is being cleaned up for more dynamic envs
like K8s.
##########
server/base/src/main/java/org/apache/accumulo/server/util/ZooZap.java:
##########
@@ -98,76 +97,70 @@ public void execute(String[] args) throws Exception {
SecurityUtil.serverLogin(siteConf);
}
- String volDir =
VolumeConfiguration.getVolumeUris(siteConf).iterator().next();
- Path instanceDir = new Path(volDir, "instance_id");
- InstanceId iid = VolumeManager.getInstanceIDFromHdfs(instanceDir, new
Configuration());
- ZooReaderWriter zoo = new ZooReaderWriter(siteConf);
-
- if (opts.zapManager) {
- String managerLockPath = Constants.ZROOT + "/" + iid +
Constants.ZMANAGER_LOCK;
-
- try {
- zapDirectory(zoo, managerLockPath, opts);
- } catch (KeeperException | InterruptedException e) {
- e.printStackTrace();
+ try (var context = new ServerContext(siteConf)) {
+ final ZooReaderWriter zoo = context.getZooReaderWriter();
+ if (opts.zapManager) {
+ ServiceLockPath managerLockPath =
context.getServerPaths().createManagerPath();
+ try {
+ zapDirectory(zoo, managerLockPath, opts);
+ } catch (KeeperException | InterruptedException e) {
+ e.printStackTrace();
+ }
}
- }
- if (opts.zapTservers) {
- String tserversPath = Constants.ZROOT + "/" + iid +
Constants.ZTSERVERS;
- try {
- List<String> children = zoo.getChildren(tserversPath);
- for (String child : children) {
- message("Deleting " + tserversPath + "/" + child + " from
zookeeper", opts);
-
- if (opts.zapManager) {
- zoo.recursiveDelete(tserversPath + "/" + child,
NodeMissingPolicy.SKIP);
- } else {
- var zLockPath = ServiceLock.path(tserversPath + "/" + child);
- if (!zoo.getChildren(zLockPath.toString()).isEmpty()) {
- if (!ServiceLock.deleteLock(zoo, zLockPath, "tserver")) {
- message("Did not delete " + tserversPath + "/" + child,
opts);
+ if (opts.zapTservers) {
+ try {
+ Set<ServiceLockPath> tserverLockPaths =
Review Comment:
Not sure about this, but this code may have been cleaning up empty lock
nodes. If it was doing that, it may not be doing that now.
##########
server/base/src/main/java/org/apache/accumulo/server/manager/LiveTServerSet.java:
##########
@@ -227,16 +229,22 @@ public synchronized void scanServers() {
try {
final Set<TServerInstance> updates = new HashSet<>();
final Set<TServerInstance> doomed = new HashSet<>();
+ final ZooCache zc = getZooCache();
+ final String tserverRoot = context.getZooKeeperRoot() +
Constants.ZTSERVERS;
- final String path = context.getZooKeeperRoot() + Constants.ZTSERVERS;
+ Set<ServiceLockPath> tservers = new HashSet<>();
- HashSet<String> all = new HashSet<>(current.keySet());
- all.addAll(getZooCache().getChildren(path));
+ for (String resourceGroup : zc.getChildren(tserverRoot)) {
Review Comment:
I made another comment suggesting that maybe all the methods to get servers
should have a third argument specifying if the path must have children lock
nodes or not.
##########
server/base/src/main/java/org/apache/accumulo/server/util/ListInstances.java:
##########
@@ -165,8 +166,8 @@ private static String getManager(ZooCache cache, InstanceId
iid, boolean printEr
}
try {
- var zLockManagerPath =
- ServiceLock.path(Constants.ZROOT + "/" + iid +
Constants.ZMANAGER_LOCK);
+ var zLockManagerPath =
ServiceLockPaths.parse(Optional.of(Constants.ZMANAGER_LOCK),
Review Comment:
This is Just a question to check an assumption. Would this be able to use
`createManagerPath()` if it had a client or server context object? Suspect
this code would call that method, but it cant because the lack of a context, is
that correct?
--
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]
