You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "dlmarion (via GitHub)" <gi...@apache.org> on 2026/01/22 21:09:06 UTC
[PR] Added ZooZap and ServiceLock to delete ScanServer locks [accumulo]
dlmarion opened a new pull request, #6073:
URL: https://github.com/apache/accumulo/pull/6073
The ScanServer locks in 2.1.x have a different format than the other locks and require different methods. The lock structure has been normalized in the planned version 4.0.
Closes #6067
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added ZooZap and ServiceLock methods to delete ScanServer locks [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #6073:
URL: https://github.com/apache/accumulo/pull/6073#discussion_r2731906576
##########
server/base/src/test/java/org/apache/accumulo/server/util/AdminTest.java:
##########
@@ -93,4 +102,114 @@ public void testCannotQualifySessionId() {
EasyMock.verify(zc);
}
+ /**
+ * SServer group filter should use lock data (UUID,group).
+ */
+ @SuppressWarnings("deprecation")
+ @Test
+ public void testSserverGroupFilterUsesLockData() throws Exception {
+
+ ZooReaderWriter zoo = EasyMock.createNiceMock(ZooReaderWriter.class);
+ ZooKeeper zk = EasyMock.createNiceMock(ZooKeeper.class);
+
+ String basePath = "/accumulo/iid/sservers";
+ String hostDefault = "host1:10000";
+ String hostOther = "host2:10001";
+ String zlock1 = "zlock#00000000-0000-0000-0000-aaaaaaaaaaaa#0000000001";
+ String zlock2 = "zlock#00000000-0000-0000-0000-bbbbbbbbbbbb#0000000001";
+
+ EasyMock.expect(zoo.exists(basePath)).andReturn(true);
+ EasyMock.expect(zoo.getChildren(basePath)).andReturn(List.of(hostDefault, hostOther));
+ EasyMock.expect(zoo.getZooKeeper()).andReturn(zk);
+ EasyMock.expect(zk.getChildren(basePath + "/" + hostDefault, null)).andReturn(List.of(zlock1));
+ EasyMock.expect(zk.getData(basePath + "/" + hostDefault + "/" + zlock1, false, null))
+ .andReturn((UUID.randomUUID().toString() + ",default").getBytes(UTF_8));
+ EasyMock.expect(zk.getChildren(basePath + "/" + hostOther, null)).andReturn(List.of(zlock2));
+ EasyMock.expect(zk.getData(basePath + "/" + hostOther + "/" + zlock2, false, null))
+ .andReturn((UUID.randomUUID().toString() + ",rg1").getBytes(UTF_8));
+
+ AtomicBoolean deletedDefault = new AtomicBoolean(false);
Review Comment:
Merged.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added ZooZap and ServiceLock methods to delete ScanServer locks [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #6073:
URL: https://github.com/apache/accumulo/pull/6073#discussion_r2722510580
##########
assemble/bin/accumulo-cluster:
##########
@@ -796,7 +796,7 @@ function prune() {
read -r -a groups <<<"$ARG_SSERVER_GROUP"
else
# find all groups known in zookeeper, this will allow pruning entire groups that do not even exist in cluster.yaml
- readarray -t groups < <(jq -r ".summaries.S_SERVER.resourceGroups | .[] " "$service_json")
+ readarray -t groups < <(jq -r ".summaries.S_SERVER.resourceGroups | keys | .[]?" "$service_json")
Review Comment:
I think this change is necessary because of the changes in https://github.com/apache/accumulo/pull/5779 to StatusSummary where the resource groups went from a Set of names to a Map of resource group names to number of hosts.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added ZooZap and ServiceLock methods to delete ScanServer locks [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #6073:
URL: https://github.com/apache/accumulo/pull/6073#discussion_r2722511326
##########
assemble/bin/accumulo-cluster:
##########
@@ -815,7 +815,7 @@ function prune() {
read -r -a groups <<<"$ARG_COMPACTOR_GROUP"
else
# find all groups known in zookeeper, this will allow pruning entire groups that do not even exist in cluster.yaml
- readarray -t groups < <(jq -r ".summaries.COMPACTOR.resourceGroups | .[] " "$service_json")
+ readarray -t groups < <(jq -r ".summaries.COMPACTOR.resourceGroups | keys | .[]?" "$service_json")
Review Comment:
Same comment here regarding #5779
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added ZooZap and ServiceLock methods to delete ScanServer locks [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #6073:
URL: https://github.com/apache/accumulo/pull/6073#discussion_r2728533261
##########
core/src/main/java/org/apache/accumulo/core/fate/zookeeper/ServiceLock.java:
##########
@@ -754,6 +755,44 @@ public static void deleteLocks(ZooReaderWriter zk, String zPath,
}
}
+ @Deprecated(since = "2.1.5")
Review Comment:
Why mark this deprecated? This package is not in the public API, so does not seem needed.
##########
core/src/main/java/org/apache/accumulo/core/fate/zookeeper/ServiceLock.java:
##########
@@ -754,6 +755,44 @@ public static void deleteLocks(ZooReaderWriter zk, String zPath,
}
}
+ @Deprecated(since = "2.1.5")
+ public static void deleteScanServerLocks(ZooReaderWriter zk, String zPath,
+ Predicate<HostAndPort> hostPortPredicate, Predicate<String> groupPredicate,
+ Consumer<String> messageOutput, Boolean dryRun) throws KeeperException, InterruptedException {
+
+ Objects.requireNonNull(zPath, "Lock path cannot be null");
+ Objects.requireNonNull(groupPredicate, "group predicate cannot be null");
+ if (!zk.exists(zPath)) {
+ throw new IllegalStateException("Path " + zPath + " does not exist");
+ }
+
+ List<String> servers = zk.getChildren(zPath);
+ if (servers.isEmpty()) {
+ throw new IllegalStateException("No server locks are held at " + zPath);
Review Comment:
Will this cause a failure if there are no scan servers? If so attempting to delete some scan servers when there are no scan servers seems like it should not fail.
##########
server/base/src/test/java/org/apache/accumulo/server/util/AdminTest.java:
##########
@@ -93,4 +102,114 @@ public void testCannotQualifySessionId() {
EasyMock.verify(zc);
}
+ /**
+ * SServer group filter should use lock data (UUID,group).
+ */
+ @SuppressWarnings("deprecation")
+ @Test
+ public void testSserverGroupFilterUsesLockData() throws Exception {
+
+ ZooReaderWriter zoo = EasyMock.createNiceMock(ZooReaderWriter.class);
+ ZooKeeper zk = EasyMock.createNiceMock(ZooKeeper.class);
+
+ String basePath = "/accumulo/iid/sservers";
+ String hostDefault = "host1:10000";
+ String hostOther = "host2:10001";
+ String zlock1 = "zlock#00000000-0000-0000-0000-aaaaaaaaaaaa#0000000001";
+ String zlock2 = "zlock#00000000-0000-0000-0000-bbbbbbbbbbbb#0000000001";
+
+ EasyMock.expect(zoo.exists(basePath)).andReturn(true);
+ EasyMock.expect(zoo.getChildren(basePath)).andReturn(List.of(hostDefault, hostOther));
+ EasyMock.expect(zoo.getZooKeeper()).andReturn(zk);
+ EasyMock.expect(zk.getChildren(basePath + "/" + hostDefault, null)).andReturn(List.of(zlock1));
+ EasyMock.expect(zk.getData(basePath + "/" + hostDefault + "/" + zlock1, false, null))
+ .andReturn((UUID.randomUUID().toString() + ",default").getBytes(UTF_8));
+ EasyMock.expect(zk.getChildren(basePath + "/" + hostOther, null)).andReturn(List.of(zlock2));
+ EasyMock.expect(zk.getData(basePath + "/" + hostOther + "/" + zlock2, false, null))
+ .andReturn((UUID.randomUUID().toString() + ",rg1").getBytes(UTF_8));
+
+ AtomicBoolean deletedDefault = new AtomicBoolean(false);
Review Comment:
Rather than using these booleans could strict mocks be used instead that fail if any unexpected `recursiveDelete` call is seen?
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added ZooZap and ServiceLock methods to delete ScanServer locks [accumulo]
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr commented on code in PR #6073:
URL: https://github.com/apache/accumulo/pull/6073#discussion_r2722459091
##########
server/base/src/main/java/org/apache/accumulo/server/util/ZooZap.java:
##########
@@ -245,7 +245,7 @@ public void zap(SiteConfiguration siteConf, String... args) {
if (opts.zapScanServers) {
String sserversPath = Constants.ZROOT + "/" + iid + Constants.ZSSERVERS;
try {
- removeGroupedLocks(zoo, sserversPath, groupPredicate, hostPortPredicate, opts);
+ removeScanServerGroupLocks(zoo, sserversPath, hostPortPredicate, groupPredicate, opts);
Review Comment:
(feel free to ignore): `removeScanServerGroupLocks` and `removeGroupedLocks` take the same args but in different orders.
##########
server/base/src/main/java/org/apache/accumulo/server/util/ZooZap.java:
##########
Review Comment:
```suggestion
static void removeCompactorGroupedLocks(ZooReaderWriter zoo, String path, Predicate<String> groupPredicate,
```
This method does not exist in `main` and is only used by compactors so rename it to be more precise in it's usage.
##########
assemble/bin/accumulo-cluster:
##########
@@ -796,7 +796,7 @@ function prune() {
read -r -a groups <<<"$ARG_SSERVER_GROUP"
else
# find all groups known in zookeeper, this will allow pruning entire groups that do not even exist in cluster.yaml
- readarray -t groups < <(jq -r ".summaries.S_SERVER.resourceGroups | .[] " "$service_json")
+ readarray -t groups < <(jq -r ".summaries.S_SERVER.resourceGroups | keys | .[]?" "$service_json")
Review Comment:
Was this related to the fix or just handled a different error case when resourceGroups aren't present?
##########
server/tserver/src/main/java/org/apache/accumulo/tserver/ScanServer.java:
##########
@@ -328,7 +328,7 @@ public String getClientAddressString() {
}
/**
- * Set up nodes and locks in ZooKeeper for this Compactor
+ * Set up nodes and locks in ZooKeeper for this ScanServer
Review Comment:
👍
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added ZooZap and ServiceLock methods to delete ScanServer locks [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #6073:
URL: https://github.com/apache/accumulo/pull/6073#discussion_r2722710419
##########
core/src/main/java/org/apache/accumulo/core/fate/zookeeper/ServiceLock.java:
##########
@@ -754,6 +755,39 @@ public static void deleteLocks(ZooReaderWriter zk, String zPath,
}
}
+ @Deprecated(since = "2.1.5")
+ public static void deleteScanServerLocks(ZooReaderWriter zk, String zPath,
+ Predicate<HostAndPort> hostPortPredicate, Predicate<String> groupPredicate,
+ Consumer<String> messageOutput, Boolean dryRun) throws KeeperException, InterruptedException {
+
+ Objects.requireNonNull(zPath, "Lock path cannot be null");
+ Objects.requireNonNull(groupPredicate, "group predicate cannot be null");
+ if (!zk.exists(zPath)) {
+ throw new IllegalStateException("Path " + zPath + " does not exist");
+ }
+
+ List<String> servers = zk.getChildren(zPath);
+ if (servers.isEmpty()) {
+ throw new IllegalStateException("No server locks are held at " + zPath);
+ }
+
+ ZooKeeper z = zk.getZooKeeper();
+ for (String server : servers) {
+ if (hostPortPredicate.test(HostAndPort.fromString(server))) {
+ byte[] lockData = ServiceLock.getLockData(z, path(zPath + "/" + server));
+ String lockContent = new String(lockData, UTF_8);
Review Comment:
It looks like `getLockData` returns null if there are no children (no locks)
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added ZooZap and ServiceLock methods to delete ScanServer locks [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #6073:
URL: https://github.com/apache/accumulo/pull/6073#discussion_r2722738903
##########
server/base/src/main/java/org/apache/accumulo/server/util/ZooZap.java:
##########
Review Comment:
Changed method name in 49a9d1c
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added ZooZap and ServiceLock methods to delete ScanServer locks [accumulo]
Posted by "DomGarguilo (via GitHub)" <gi...@apache.org>.
DomGarguilo commented on code in PR #6073:
URL: https://github.com/apache/accumulo/pull/6073#discussion_r2722577758
##########
core/src/main/java/org/apache/accumulo/core/fate/zookeeper/ServiceLock.java:
##########
@@ -754,6 +755,39 @@ public static void deleteLocks(ZooReaderWriter zk, String zPath,
}
}
+ @Deprecated(since = "2.1.5")
+ public static void deleteScanServerLocks(ZooReaderWriter zk, String zPath,
+ Predicate<HostAndPort> hostPortPredicate, Predicate<String> groupPredicate,
+ Consumer<String> messageOutput, Boolean dryRun) throws KeeperException, InterruptedException {
+
+ Objects.requireNonNull(zPath, "Lock path cannot be null");
+ Objects.requireNonNull(groupPredicate, "group predicate cannot be null");
+ if (!zk.exists(zPath)) {
+ throw new IllegalStateException("Path " + zPath + " does not exist");
+ }
+
+ List<String> servers = zk.getChildren(zPath);
+ if (servers.isEmpty()) {
+ throw new IllegalStateException("No server locks are held at " + zPath);
+ }
+
+ ZooKeeper z = zk.getZooKeeper();
+ for (String server : servers) {
+ if (hostPortPredicate.test(HostAndPort.fromString(server))) {
+ byte[] lockData = ServiceLock.getLockData(z, path(zPath + "/" + server));
+ String lockContent = new String(lockData, UTF_8);
Review Comment:
We do `zPath + "/" + server` a lot here. Would be nice to add a var for that. Also once that var is created we should update the log message in this method to use that because I think its logging the wrong path right now.
Also, my IDE is warning that `lockData` could be null. We might want to handle that. Maybe skipping that `server` and logging or `messageOutput.accept` that we skipped it.
##########
server/base/src/main/java/org/apache/accumulo/server/util/ZooZap.java:
##########
@@ -245,7 +245,7 @@ public void zap(SiteConfiguration siteConf, String... args) {
if (opts.zapScanServers) {
String sserversPath = Constants.ZROOT + "/" + iid + Constants.ZSSERVERS;
try {
- removeGroupedLocks(zoo, sserversPath, groupPredicate, hostPortPredicate, opts);
+ removeScanServerGroupLocks(zoo, sserversPath, hostPortPredicate, groupPredicate, opts);
Review Comment:
I think we need to do something like this when `--include-groups` is not set.
```suggestion
if (opts.includeGroups == null) {
removeLocks(zoo, sserversPath, hostPortPredicate, opts);
} else {
removeScanServerGroupLocks(zoo, sserversPath, hostPortPredicate, groupPredicate, opts);
}
```
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added ZooZap and ServiceLock methods to delete ScanServer locks [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #6073:
URL: https://github.com/apache/accumulo/pull/6073#discussion_r2722737440
##########
core/src/main/java/org/apache/accumulo/core/fate/zookeeper/ServiceLock.java:
##########
@@ -754,6 +755,39 @@ public static void deleteLocks(ZooReaderWriter zk, String zPath,
}
}
+ @Deprecated(since = "2.1.5")
+ public static void deleteScanServerLocks(ZooReaderWriter zk, String zPath,
+ Predicate<HostAndPort> hostPortPredicate, Predicate<String> groupPredicate,
+ Consumer<String> messageOutput, Boolean dryRun) throws KeeperException, InterruptedException {
+
+ Objects.requireNonNull(zPath, "Lock path cannot be null");
+ Objects.requireNonNull(groupPredicate, "group predicate cannot be null");
+ if (!zk.exists(zPath)) {
+ throw new IllegalStateException("Path " + zPath + " does not exist");
+ }
+
+ List<String> servers = zk.getChildren(zPath);
+ if (servers.isEmpty()) {
+ throw new IllegalStateException("No server locks are held at " + zPath);
+ }
+
+ ZooKeeper z = zk.getZooKeeper();
+ for (String server : servers) {
+ if (hostPortPredicate.test(HostAndPort.fromString(server))) {
+ byte[] lockData = ServiceLock.getLockData(z, path(zPath + "/" + server));
+ String lockContent = new String(lockData, UTF_8);
Review Comment:
Fixed in 49a9d1c
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added ZooZap and ServiceLock methods to delete ScanServer locks [accumulo]
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr commented on PR #6073:
URL: https://github.com/apache/accumulo/pull/6073#issuecomment-3799941028
Tested this locally and scan servers now prune correctly with the group keyword working correctly.
While testing I also noticed that the GC lock was always being marked for pruning.
Went back to 2.1.4 and that behavior was present before this PR.
Looks like the GC lock format is different than the manager or monitor lock so the hostPortPredicate test never passes.
The gc lock format is `GC_CLIENT=localhost:9998` vs the manager's `localhost:9999`.
Assuming we don't want to change the lock format, the fix is just adding another specific lock type deletion method that does the following check:
```
var lockData = ServiceLock.getLockData(zoo.getZooKeeper(), ServiceLock.path(path));
if (lockData != null) {
String lockContent = new String(lockData, UTF_8);
String[] parts = lockContent.split("=");
if (parts.length == 2 && hostPortPredicate.test(HostAndPort.fromString(parts[1]))) {
zapDirectory(zoo, path, ops);
}
}
```
I'll create an issue ticket for that work. I think this PR is good to merge.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added ZooZap and ServiceLock methods to delete ScanServer locks [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion merged PR #6073:
URL: https://github.com/apache/accumulo/pull/6073
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added ZooZap and ServiceLock methods to delete ScanServer locks [accumulo]
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr commented on code in PR #6073:
URL: https://github.com/apache/accumulo/pull/6073#discussion_r2722682930
##########
core/src/main/java/org/apache/accumulo/core/fate/zookeeper/ServiceLock.java:
##########
@@ -754,6 +755,39 @@ public static void deleteLocks(ZooReaderWriter zk, String zPath,
}
}
+ @Deprecated(since = "2.1.5")
+ public static void deleteScanServerLocks(ZooReaderWriter zk, String zPath,
+ Predicate<HostAndPort> hostPortPredicate, Predicate<String> groupPredicate,
+ Consumer<String> messageOutput, Boolean dryRun) throws KeeperException, InterruptedException {
+
+ Objects.requireNonNull(zPath, "Lock path cannot be null");
+ Objects.requireNonNull(groupPredicate, "group predicate cannot be null");
+ if (!zk.exists(zPath)) {
+ throw new IllegalStateException("Path " + zPath + " does not exist");
+ }
+
+ List<String> servers = zk.getChildren(zPath);
+ if (servers.isEmpty()) {
+ throw new IllegalStateException("No server locks are held at " + zPath);
+ }
+
+ ZooKeeper z = zk.getZooKeeper();
+ for (String server : servers) {
+ if (hostPortPredicate.test(HostAndPort.fromString(server))) {
+ byte[] lockData = ServiceLock.getLockData(z, path(zPath + "/" + server));
+ String lockContent = new String(lockData, UTF_8);
Review Comment:
getLockData looks like it either returns an Optional.empty() or valid data.
ServiceLockData.parse seems to handle null checks so this might just need some optional handling.
Agree that `zPath + "/" + server` would be nice. I see the path log message issue on 783
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added ZooZap and ServiceLock methods to delete ScanServer locks [accumulo]
Posted by "DomGarguilo (via GitHub)" <gi...@apache.org>.
DomGarguilo commented on code in PR #6073:
URL: https://github.com/apache/accumulo/pull/6073#discussion_r2722730626
##########
server/base/src/main/java/org/apache/accumulo/server/util/ZooZap.java:
##########
@@ -245,7 +245,7 @@ public void zap(SiteConfiguration siteConf, String... args) {
if (opts.zapScanServers) {
String sserversPath = Constants.ZROOT + "/" + iid + Constants.ZSSERVERS;
try {
- removeGroupedLocks(zoo, sserversPath, groupPredicate, hostPortPredicate, opts);
+ removeScanServerGroupLocks(zoo, sserversPath, hostPortPredicate, groupPredicate, opts);
Review Comment:
I think it will work. If --include-groups is not set we can skip the extra logic altogether though. Not a huge deal but avoids a bit of work/filtering when we already know we dont need to it.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added ZooZap and ServiceLock methods to delete ScanServer locks [accumulo]
Posted by "DomGarguilo (via GitHub)" <gi...@apache.org>.
DomGarguilo commented on code in PR #6073:
URL: https://github.com/apache/accumulo/pull/6073#discussion_r2728808196
##########
server/base/src/test/java/org/apache/accumulo/server/util/AdminTest.java:
##########
@@ -93,4 +102,114 @@ public void testCannotQualifySessionId() {
EasyMock.verify(zc);
}
+ /**
+ * SServer group filter should use lock data (UUID,group).
+ */
+ @SuppressWarnings("deprecation")
+ @Test
+ public void testSserverGroupFilterUsesLockData() throws Exception {
+
+ ZooReaderWriter zoo = EasyMock.createNiceMock(ZooReaderWriter.class);
+ ZooKeeper zk = EasyMock.createNiceMock(ZooKeeper.class);
+
+ String basePath = "/accumulo/iid/sservers";
+ String hostDefault = "host1:10000";
+ String hostOther = "host2:10001";
+ String zlock1 = "zlock#00000000-0000-0000-0000-aaaaaaaaaaaa#0000000001";
+ String zlock2 = "zlock#00000000-0000-0000-0000-bbbbbbbbbbbb#0000000001";
+
+ EasyMock.expect(zoo.exists(basePath)).andReturn(true);
+ EasyMock.expect(zoo.getChildren(basePath)).andReturn(List.of(hostDefault, hostOther));
+ EasyMock.expect(zoo.getZooKeeper()).andReturn(zk);
+ EasyMock.expect(zk.getChildren(basePath + "/" + hostDefault, null)).andReturn(List.of(zlock1));
+ EasyMock.expect(zk.getData(basePath + "/" + hostDefault + "/" + zlock1, false, null))
+ .andReturn((UUID.randomUUID().toString() + ",default").getBytes(UTF_8));
+ EasyMock.expect(zk.getChildren(basePath + "/" + hostOther, null)).andReturn(List.of(zlock2));
+ EasyMock.expect(zk.getData(basePath + "/" + hostOther + "/" + zlock2, false, null))
+ .andReturn((UUID.randomUUID().toString() + ",rg1").getBytes(UTF_8));
+
+ AtomicBoolean deletedDefault = new AtomicBoolean(false);
Review Comment:
I was making a small PR against this branch for this test so included a commit in that to do this too
https://github.com/dlmarion/accumulo/pull/58
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added ZooZap and ServiceLock methods to delete ScanServer locks [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #6073:
URL: https://github.com/apache/accumulo/pull/6073#discussion_r2731925459
##########
core/src/main/java/org/apache/accumulo/core/fate/zookeeper/ServiceLock.java:
##########
@@ -754,6 +755,44 @@ public static void deleteLocks(ZooReaderWriter zk, String zPath,
}
}
+ @Deprecated(since = "2.1.5")
+ public static void deleteScanServerLocks(ZooReaderWriter zk, String zPath,
+ Predicate<HostAndPort> hostPortPredicate, Predicate<String> groupPredicate,
+ Consumer<String> messageOutput, Boolean dryRun) throws KeeperException, InterruptedException {
+
+ Objects.requireNonNull(zPath, "Lock path cannot be null");
+ Objects.requireNonNull(groupPredicate, "group predicate cannot be null");
+ if (!zk.exists(zPath)) {
+ throw new IllegalStateException("Path " + zPath + " does not exist");
+ }
+
+ List<String> servers = zk.getChildren(zPath);
+ if (servers.isEmpty()) {
+ throw new IllegalStateException("No server locks are held at " + zPath);
Review Comment:
This follows the same pattern as `deleteLocks`.
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added ZooZap and ServiceLock methods to delete ScanServer locks [accumulo]
Posted by "ddanielr (via GitHub)" <gi...@apache.org>.
ddanielr commented on code in PR #6073:
URL: https://github.com/apache/accumulo/pull/6073#discussion_r2722696723
##########
server/base/src/main/java/org/apache/accumulo/server/util/ZooZap.java:
##########
@@ -245,7 +245,7 @@ public void zap(SiteConfiguration siteConf, String... args) {
if (opts.zapScanServers) {
String sserversPath = Constants.ZROOT + "/" + iid + Constants.ZSSERVERS;
try {
- removeGroupedLocks(zoo, sserversPath, groupPredicate, hostPortPredicate, opts);
+ removeScanServerGroupLocks(zoo, sserversPath, hostPortPredicate, groupPredicate, opts);
Review Comment:
I don't think that's necessary. If the resource group is not defined then the GroupPredicate is set to true.
https://github.com/apache/accumulo/blob/e689a8fb7133464f08e5d69623d199703ce7c8a7/server/base/src/main/java/org/apache/accumulo/server/util/ZooZap.java#L146-L152
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Added ZooZap and ServiceLock methods to delete ScanServer locks [accumulo]
Posted by "dlmarion (via GitHub)" <gi...@apache.org>.
dlmarion commented on code in PR #6073:
URL: https://github.com/apache/accumulo/pull/6073#discussion_r2722696800
##########
server/base/src/main/java/org/apache/accumulo/server/util/ZooZap.java:
##########
@@ -245,7 +245,7 @@ public void zap(SiteConfiguration siteConf, String... args) {
if (opts.zapScanServers) {
String sserversPath = Constants.ZROOT + "/" + iid + Constants.ZSSERVERS;
try {
- removeGroupedLocks(zoo, sserversPath, groupPredicate, hostPortPredicate, opts);
+ removeScanServerGroupLocks(zoo, sserversPath, hostPortPredicate, groupPredicate, opts);
Review Comment:
When `--include-groups` is not set, then the group predicate is set to just `true`, which should match all groups. This is done on line 151 above. Do you think that will not work?
--
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-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org