Skip to content

Added ZooZap and ServiceLock methods to delete ScanServer locks #6073

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
dlmarion wants to merge 7 commits into
base: 2.1
Choose a base branch
from
Open

Added ZooZap and ServiceLock methods to delete ScanServer locks #6073

dlmarion wants to merge 7 commits into from
+133 −9

Conversation

@dlmarion
Copy link
Contributor

@dlmarion dlmarion commented Jan 22, 2026

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

@dlmarion @DomGarguilo
Added ZooZap and ServiceLock to delete ScanServer locks
859cda2 
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 apache#6067

Co-authored-by: Dom G. <domgarguilo@apache.org>
@dlmarion dlmarion added this to the 2.1.5 milestone Jan 22, 2026
@dlmarion dlmarion self-assigned this Jan 22, 2026
@dlmarion dlmarion changed the title Added ZooZap and ServiceLock to delete ScanServer locks Added ZooZap and ServiceLock methods to delete ScanServer locks Jan 22, 2026
dlmarion
dlmarion commented Jan 23, 2026
View reviewed changes
dlmarion
dlmarion commented Jan 23, 2026
View reviewed changes
dlmarion
dlmarion commented Jan 23, 2026
View reviewed changes
Copy link
Contributor Author

@dlmarion dlmarion left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

added notes about why jq changes are needed.

@dlmarion dlmarion linked an issue Jan 23, 2026 that may be closed by this pull request
accumulo-cluster prune option does not work with scan servers #6067
Open
DomGarguilo
DomGarguilo requested changes Jan 23, 2026
View reviewed changes
ddanielr
ddanielr approved these changes Jan 23, 2026
View reviewed changes
Copy link
Contributor

@ddanielr ddanielr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes look good. Minor comments.

server/base/src/main/java/org/apache/accumulo/server/util/ZooZap.java Outdated
String sserversPath = Constants.ZROOT + "/" + iid + Constants.ZSSERVERS;
try {
removeGroupedLocks(zoo, sserversPath, groupPredicate, hostPortPredicate, opts);
removeScanServerGroupLocks(zoo, sserversPath, hostPortPredicate, groupPredicate, opts);
Copy link
Contributor

@ddanielr ddanielr Jan 23, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(feel free to ignore): removeScanServerGroupLocks and removeGroupedLocks take the same args but in different orders.

@ddanielr
Copy link
Contributor

ddanielr commented Jan 26, 2026

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.

@ddanielr ddanielr mentioned this pull request Jan 26, 2026
Open
keith-turner
keith-turner reviewed Jan 26, 2026
View reviewed changes
core/src/main/java/org/apache/accumulo/core/fate/zookeeper/ServiceLock.java

List<String> servers = zk.getChildren(zPath);
if (servers.isEmpty()) {
throw new IllegalStateException("No server locks are held at " + zPath);
Copy link
Contributor

@keith-turner keith-turner Jan 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@dlmarion dlmarion Jan 27, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This follows the same pattern as deleteLocks.

core/src/main/java/org/apache/accumulo/core/fate/zookeeper/ServiceLock.java Outdated
}
}

@Deprecated(since = "2.1.5")
Copy link
Contributor

@keith-turner keith-turner Jan 26, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why mark this deprecated? This package is not in the public API, so does not seem needed.

@dlmarion dlmarion mentioned this pull request Jan 27, 2026
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@keith-turner keith-turner keith-turner left review comments

@ddanielr ddanielr ddanielr approved these changes

@DomGarguilo DomGarguilo DomGarguilo requested changes

Assignees

@dlmarion dlmarion

Labels

None yet

Projects

None yet

Milestone

2.1.5

Development

Successfully merging this pull request may close these issues.

accumulo-cluster prune option does not work with scan servers

4 participants

@dlmarion @ddanielr @keith-turner @DomGarguilo