speeds up fate lock acquisition #5262

keith-turner · 2025-01-15T00:38:32Z

Stores the lock data for fate locks in the zookeeper node name instead of the zookeeper data for the node. Ran some local performance test with hundreds of fate operations and saw lock times go from 750ms to 15ms.

fixes #5181

Stores the lock data for fate locks in the zookeeper node name instead of the zookeeper data for the node. Ran some local performance test with hundreds of fate operations and saw lock times go from 750ms to 15ms. fixes apache#5181

keith-turner · 2025-01-15T00:40:22Z

core/src/main/java/org/apache/accumulo/core/fate/zookeeper/FateLock.java

+    }
+  }
+
+  // TODO change data arg from byte[] to String.. in the rest of the code its always a String.


I will open a follow on issue for this and remove the TODO before merging.

Removed the TODO and #5264. In the TODO I was thinking of using String, but realized using more concrete types would be better.

ctubbsii · 2025-01-15T00:47:31Z

core/src/main/java/org/apache/accumulo/core/fate/AdminUtil.java

+        List<FateLock.FateLockNode> lockNodes =
+            FateLock.validateAndWarn(fLockPath, zr.getChildren(fLockPath.toString()));
+
+        lockNodes.sort(Comparator.comparingLong(ln -> ln.sequence));


Should the return type be a sorted set instead of a list that you sort later?

Updated to a sorted set in 3f25ce0

core/src/main/java/org/apache/accumulo/core/fate/zookeeper/DistributedReadWriteLock.java

core/src/main/java/org/apache/accumulo/core/fate/zookeeper/FateLock.java

…eLock.java Co-authored-by: Christopher Tubbs <ctubbsii@apache.org>

…tributedReadWriteLock.java Co-authored-by: Christopher Tubbs <ctubbsii@apache.org>

keith-turner · 2025-01-15T18:20:29Z

test/src/main/java/org/apache/accumulo/test/compaction/ExternalCompaction_1_IT.java

+
+      List<TabletId> tabletIds;
+      // start a compaction on each tablet
+      try (var tablets = client.tableOperations().getTabletInformation(table1, new Range())) {


I wrote this test initially to check performance, but realized its a good test of many compaction on the same table w/ different config also.

I did not initially see a performance difference w/ this test. Looking into it the reason was the compaction were finishing too quickly, so only 8 or 9 compaction fate operations were active at a time. For the performance bug to be observed needed many more active fate operations. Locally I modified the test to kill the compactor processes and start 1K compaction fate operations. With those changes I could see orders of magnitude diffs for fate lock acquisition time.

dlmarion · 2025-01-17T17:07:41Z

core/src/main/java/org/apache/accumulo/core/fate/zookeeper/FateLock.java

+      Preconditions.checkArgument(nodeName.startsWith(PREFIX) && nodeName.charAt(len - 11) == '#',
+          "Illegal node name %s", nodeName);
+      sequence = Long.parseLong(nodeName.substring(len - 10));
+      lockData = nodeName.substring(PREFIX.length(), len - 11);


So, while this is probably unlikely to happen, the sequential numbering can overflow causing a negative to occur, which I think would cause a problem with this parsing instead of splitting on #.

Added handing and test for negative seq numbers in 2309b9b

dlmarion · 2025-01-24T14:48:56Z

core/src/main/java/org/apache/accumulo/core/fate/zookeeper/FateLock.java

      Preconditions.checkArgument(nodeName.startsWith(PREFIX) && nodeName.charAt(len - 11) == '#',
          "Illegal node name %s", nodeName);


In the case of an overflow, the character at (len - 11) be a minus sign and cause this to fail.

It depends on the number of digits in the negative number. If its a 10 digit negative number then %010d will produce a 11 char string. If its 9 digits or less then %010d will produce a 10 char string including the negative. For the 9 digit or less case the # check will not catch negative, but the later Long.parseUnsignedLong call will catch it.

For the case Long.parseUnsignedLong catches the problem it throws a more specific exception, I updated the test to check for that.

keith-turner · 2025-02-01T18:20:51Z

core/src/main/java/org/apache/accumulo/core/fate/AdminUtil.java

              locks = heldLocks;
            } else {
              locks = waitingLocks;
            }

-            locks.computeIfAbsent(fateId, k -> new ArrayList<>()).add(lda[0].charAt(0) + ":" + id);
+            locks.computeIfAbsent(fateId, k -> new ArrayList<>())


I suspect the new FateLockEntry type could be used in this Admin class instead of the string things its doing here. Going to open a follow on issue.

cshannon

Latest changes LGTM

cshannon · 2025-02-01T18:22:08Z

core/src/main/java/org/apache/accumulo/core/fate/zookeeper/FateLock.java

-      System.arraycopy(fateIdBytes, 0, result, typeBytes.length + 1, fateIdBytes.length);
-      return result;
+    public String serialize() {
+      return lockType.name() + ":" + fateId.canonical();


This is SO much nicer, not having to do all that manual effort to copy bytes around is great.

cshannon · 2025-02-01T18:24:18Z

core/src/main/java/org/apache/accumulo/core/fate/zookeeper/FateLock.java

-      this.fateId =
-          FateId.from(new String(Arrays.copyOfRange(entry, split + 1, entry.length), UTF_8));
+    private FateLockEntry(String entry) {
+      var fields = entry.split(":", 2);


Any reason to verify the number of fields is 2? I suppose it will just fail a couple lines later if not correct.

That just grabs the first two splits even if there are more. The FateId also has a : so calling split w/o the 2 will return three fields.

cshannon · 2025-02-01T18:32:51Z

core/src/main/java/org/apache/accumulo/core/fate/zookeeper/FateLock.java

-          // ignored
+        var parsed = new NodeName(name);
+        if (predicate.test(parsed.sequence, parsed.fateLockEntry)) {
+          // Use a supplier so we don't need to deserialize unless the calling code cares about


You may want to move this comment to NodeName where the supplier is created now, but it doesn't matter too much either way probably

speeds up fate lock acquisition

20fbbe8

Stores the lock data for fate locks in the zookeeper node name instead of the zookeeper data for the node. Ran some local performance test with hundreds of fate operations and saw lock times go from 750ms to 15ms. fixes apache#5181

keith-turner commented Jan 15, 2025

View reviewed changes

ctubbsii modified the milestones: 2.1.4, 4.0.0 Jan 15, 2025

ctubbsii reviewed Jan 15, 2025

View reviewed changes

keith-turner and others added 5 commits January 14, 2025 20:26

Update core/src/main/java/org/apache/accumulo/core/fate/zookeeper/Fat…

20097d5

…eLock.java Co-authored-by: Christopher Tubbs <ctubbsii@apache.org>

Update core/src/main/java/org/apache/accumulo/core/fate/zookeeper/Dis…

a45b3ad

…tributedReadWriteLock.java Co-authored-by: Christopher Tubbs <ctubbsii@apache.org>

format code

3fbdd6e

code review update

3f25ce0

remove TODO

95fb8dc

keith-turner commented Jan 15, 2025

View reviewed changes

dlmarion reviewed Jan 17, 2025

View reviewed changes

code review update

2309b9b

dlmarion reviewed Jan 24, 2025

View reviewed changes

keith-turner added 2 commits January 24, 2025 17:13

improve test

ac9a6b5

use variable

1d4d7f2

keith-turner mentioned this pull request Feb 1, 2025

Refactor Fate lock code to use concrete types #5298

Merged

keith-turner added 2 commits February 1, 2025 17:12

Merge branch 'main' into accumulo-5181

43651d3

follow on changes after merge

1c08a34

keith-turner commented Feb 1, 2025

View reviewed changes

cshannon approved these changes Feb 1, 2025

View reviewed changes

move comment

33e9bf6

keith-turner merged commit 3213646 into apache:main Feb 1, 2025
8 checks passed

keith-turner deleted the accumulo-5181 branch February 1, 2025 21:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

speeds up fate lock acquisition #5262

speeds up fate lock acquisition #5262

keith-turner commented Jan 15, 2025

keith-turner Jan 15, 2025

keith-turner Jan 15, 2025

ctubbsii Jan 15, 2025

keith-turner Jan 15, 2025

keith-turner Jan 15, 2025

dlmarion Jan 17, 2025

keith-turner Jan 17, 2025

dlmarion Jan 24, 2025

keith-turner Jan 24, 2025

keith-turner Jan 24, 2025

keith-turner Feb 1, 2025

cshannon left a comment

cshannon Feb 1, 2025

cshannon Feb 1, 2025

keith-turner Feb 1, 2025

cshannon Feb 1, 2025

		Preconditions.checkArgument(nodeName.startsWith(PREFIX) && nodeName.charAt(len - 11) == '#',
		"Illegal node name %s", nodeName);

speeds up fate lock acquisition #5262

speeds up fate lock acquisition #5262

Conversation

keith-turner commented Jan 15, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cshannon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment