You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by "keith-turner (via GitHub)" <gi...@apache.org> on 2025/08/08 16:54:27 UTC
[PR] adds ranges to table locks [accumulo]
keith-turner opened a new pull request, #5786:
URL: https://github.com/apache/accumulo/pull/5786
Added ranges to table locks using the ranges from fate operations. For write locks these ranges are widened to the nearest splits, see comments in Utils.reserveTable for details. See comments and code in DistributedReadWriteLock.WriteLock.tryLock() and
DistributedReadWriteLock.ReadLock.tryLock() for details about how these ranges change lock behavior. Added tests to excercise this new locking behavior. Modified the `accumulo admin fate -l` command to display lock ranges. Also modified how this command determines if a lock is held or waiting.
fixes #2483
--
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] adds ranges to table locks [accumulo]
Posted by "kevinrr888 (via GitHub)" <gi...@apache.org>.
kevinrr888 commented on code in PR #5786:
URL: https://github.com/apache/accumulo/pull/5786#discussion_r2277314300
##########
core/src/main/java/org/apache/accumulo/core/fate/zookeeper/FateLock.java:
##########
@@ -154,26 +176,14 @@ public static class NodeName implements Comparable<NodeName> {
.memoize(() -> FateLockEntry.deserialize(nodeName.substring(PREFIX.length(), len - 11)));
}
- @Override
- public int compareTo(NodeName o) {
- int cmp = Long.compare(sequence, o.sequence);
- if (cmp == 0) {
- cmp = fateLockEntry.get().compareTo(o.fateLockEntry.get());
- }
- return cmp;
- }
-
@Override
public boolean equals(Object o) {
- if (o instanceof NodeName) {
- return this.compareTo((NodeName) o) == 0;
- }
- return false;
+ throw new UnsupportedOperationException();
}
@Override
public int hashCode() {
- return Objects.hash(sequence, fateLockEntry.get());
+ throw new UnsupportedOperationException();
Review Comment:
why are these unsupported?
##########
core/src/test/java/org/apache/accumulo/core/fate/zookeeper/DistributedReadWriteLockTest.java:
##########
@@ -146,10 +149,69 @@ public void testLock() throws Exception {
}
}
+ private DistributedLock recover(QueueLock queueLock, FateId fateId, LockRange expectedRange,
+ LockType expectedType) {
+ var dlock = DistributedReadWriteLock.recoverLock(queueLock, fateId);
+ assertEquals(expectedRange, dlock.getRange());
+ assertEquals(expectedType, dlock.getType());
+ return dlock;
+ }
+
+ @Test
+ public void testRanges() {
Review Comment:
You may already have this testing so ignore if I'm missing it, but it would be good to test locking at the boundaries of the ranges. For example, using the existing (E, M]. Could test locking for that range, then try to lock for range (foo, E] which should succeed and try to lock for range (M,bar] which should also succeed.
##########
core/src/main/java/org/apache/accumulo/core/fate/zookeeper/FateLock.java:
##########
@@ -68,19 +72,22 @@ public String toString() {
}
}
- public static class FateLockEntry implements Comparable<FateLockEntry> {
+ public static class FateLockEntry {
final LockType lockType;
final FateId fateId;
+ final LockRange range;
- private FateLockEntry(LockType lockType, FateId fateId) {
+ private FateLockEntry(LockType lockType, FateId fateId, LockRange range) {
this.lockType = Objects.requireNonNull(lockType);
this.fateId = Objects.requireNonNull(fateId);
+ this.range = range;
}
private FateLockEntry(String entry) {
- var fields = entry.split(":", 2);
+ var fields = entry.split("_", 4);
Review Comment:
I like the switch to "_" since fateId uses ":". Could maybe put this in a static var "delimiter"
##########
core/src/main/java/org/apache/accumulo/core/fate/zookeeper/LockRange.java:
##########
@@ -0,0 +1,94 @@
+/*
+ * 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.fate.zookeeper;
+
+import org.apache.accumulo.core.data.TableId;
+import org.apache.accumulo.core.dataImpl.KeyExtent;
+import org.apache.hadoop.io.Text;
+
+public class LockRange {
+ final KeyExtent range;
Review Comment:
```suggestion
private final KeyExtent range;
```
Suggesting to avoid exposing the TableId.of("0") which has no significance
##########
core/src/main/java/org/apache/accumulo/core/fate/zookeeper/FateLock.java:
##########
@@ -251,7 +261,7 @@ public void removeEntry(FateLockEntry data, long entry) {
public static SortedSet<NodeName> validateAndWarn(FateLockPath path, List<String> children) {
log.trace("validating and sorting children at path {}", path);
- SortedSet<NodeName> validChildren = new TreeSet<>();
+ SortedSet<NodeName> validChildren = new TreeSet<>(Comparator.comparingLong(nn -> nn.sequence));
Review Comment:
What is the benefit of using a custom comparator here instead of keeping NodeName as a comparable?
##########
core/src/main/java/org/apache/accumulo/core/fate/zookeeper/FateLock.java:
##########
@@ -91,42 +98,57 @@ public FateId getFateId() {
return fateId;
}
- public String serialize() {
- return lockType.name() + ":" + fateId.canonical();
+ public LockRange getRange() {
+ return range;
}
- @Override
- public boolean equals(Object o) {
- if (o == null || getClass() != o.getClass()) {
- return false;
+ private String encode(Text row) {
Review Comment:
```suggestion
private String encodeRow(Text row) {
```
##########
core/src/main/java/org/apache/accumulo/core/fate/zookeeper/FateLock.java:
##########
@@ -91,42 +98,57 @@ public FateId getFateId() {
return fateId;
}
- public String serialize() {
- return lockType.name() + ":" + fateId.canonical();
+ public LockRange getRange() {
+ return range;
}
- @Override
- public boolean equals(Object o) {
- if (o == null || getClass() != o.getClass()) {
- return false;
+ private String encode(Text row) {
+ if (row == null) {
+ return "N";
+ } else {
+ return "P" + Base64.getEncoder().encodeToString(TextUtil.getBytes(row));
}
+ }
- FateLockEntry lockEntry = (FateLockEntry) o;
- return lockType == lockEntry.lockType && fateId.equals(lockEntry.fateId);
+ private Text decode(String enc) {
Review Comment:
```suggestion
private Text decodeRow(String enc) {
```
##########
server/manager/src/main/java/org/apache/accumulo/manager/tableOps/bulkVer2/ComputeBulkRange.java:
##########
@@ -0,0 +1,75 @@
+/*
+ * 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.manager.tableOps.bulkVer2;
+
+import java.util.Optional;
+
+import org.apache.accumulo.core.clientImpl.bulk.BulkSerialize;
+import org.apache.accumulo.core.clientImpl.bulk.LoadMappingIterator;
+import org.apache.accumulo.core.data.TableId;
+import org.apache.accumulo.core.dataImpl.KeyExtent;
+import org.apache.accumulo.core.fate.FateId;
+import org.apache.accumulo.core.fate.Repo;
+import org.apache.accumulo.manager.Manager;
+import org.apache.accumulo.manager.tableOps.ManagerRepo;
+import org.apache.accumulo.server.fs.VolumeManager;
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.io.Text;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class ComputeBulkRange extends ManagerRepo {
+ private static final long serialVersionUID = 1L;
+
+ private static final Logger log = LoggerFactory.getLogger(ComputeBulkRange.class);
+
+ private final BulkInfo bulkInfo;
+
+ public ComputeBulkRange(TableId tableId, String sourceDir, boolean setTime) {
+ BulkInfo info = new BulkInfo();
+ info.tableId = tableId;
+ info.sourceDir = sourceDir;
+ info.setTime = setTime;
+ this.bulkInfo = info;
Review Comment:
```suggestion
info.tableId = tableId;
info.sourceDir = sourceDir;
info.setTime = setTime;
this.bulkInfo = new BulkInfo();
```
--
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] adds ranges to table locks [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #5786:
URL: https://github.com/apache/accumulo/pull/5786#discussion_r2305174470
##########
core/src/main/java/org/apache/accumulo/core/fate/AdminUtil.java:
##########
@@ -417,9 +430,10 @@ public void print(Map<FateInstanceType,ReadOnlyFateStore<T>> readOnlyFateStores,
for (TransactionStatus txStatus : fateStatus.getTransactions()) {
fmt.format(
- "%-15s fateId: %s status: %-18s locked: %-15s locking: %-15s op: %-15s created: %s%n",
+ "%-15s fateId: %s status: %-18s locked: %-15s locking: %-15s op: %-15s created: %s lock range: %s%n",
txStatus.getFateOp(), txStatus.getFateId(), txStatus.getStatus(), txStatus.getHeldLocks(),
- txStatus.getWaitingLocks(), txStatus.getTop(), txStatus.getTimeCreatedFormatted());
+ txStatus.getWaitingLocks(), txStatus.getTop(), txStatus.getTimeCreatedFormatted(),
+ txStatus.getLockRange());
Review Comment:
I did not see any changes that were needed.
--
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] adds ranges to table locks [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #5786:
URL: https://github.com/apache/accumulo/pull/5786#discussion_r2305181264
##########
core/src/main/java/org/apache/accumulo/core/fate/zookeeper/FateLock.java:
##########
@@ -251,7 +264,7 @@ public void removeEntry(FateLockEntry data, long entry) {
public static SortedSet<NodeName> validateAndWarn(FateLockPath path, List<String> children) {
log.trace("validating and sorting children at path {}", path);
- SortedSet<NodeName> validChildren = new TreeSet<>();
+ SortedSet<NodeName> validChildren = new TreeSet<>(Comparator.comparingLong(nn -> nn.sequence));
Review Comment:
I think this is ok because it not called too frequently.
--
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] adds ranges to table locks [accumulo]
Posted by "kevinrr888 (via GitHub)" <gi...@apache.org>.
kevinrr888 commented on code in PR #5786:
URL: https://github.com/apache/accumulo/pull/5786#discussion_r2293955212
##########
core/src/main/java/org/apache/accumulo/core/fate/zookeeper/FateLock.java:
##########
@@ -251,7 +261,7 @@ public void removeEntry(FateLockEntry data, long entry) {
public static SortedSet<NodeName> validateAndWarn(FateLockPath path, List<String> children) {
log.trace("validating and sorting children at path {}", path);
- SortedSet<NodeName> validChildren = new TreeSet<>();
+ SortedSet<NodeName> validChildren = new TreeSet<>(Comparator.comparingLong(nn -> nn.sequence));
Review Comment:
> Adding compareTo method to the class should compare on all fields in the class, which adds a lot of complexity to the code that is not needed
Good point. Was worried about possible uses of NameNode in some sorted collection, with dev not realizing that they aren't comparable. But my IDE warns when you are using a sorted collection with non-comparable elts, and it falls more on the dev to understand the class they are working with.
So, I agree and think current impl is good
--
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] adds ranges to table locks [accumulo]
Posted by "kevinrr888 (via GitHub)" <gi...@apache.org>.
kevinrr888 commented on code in PR #5786:
URL: https://github.com/apache/accumulo/pull/5786#discussion_r2304856364
##########
server/manager/src/main/java/org/apache/accumulo/manager/tableOps/Utils.java:
##########
@@ -80,17 +91,142 @@ public static <T extends AbstractId<T>> T getNextId(String name, ServerContext c
static final Lock tableNameLock = new ReentrantLock();
+ private static KeyExtent findContaining(Ample ample, TableId tableId, Text row) {
+ Objects.requireNonNull(row);
+ try (var tablets = ample.readTablets().forTable(tableId).overlapping(row, true, row)
+ .fetch(TabletMetadata.ColumnType.PREV_ROW).build()) {
+ return tablets.stream().collect(MoreCollectors.onlyElement()).getExtent();
+ }
+ }
+
+ /**
+ * Widen a range to the greatest table split that is before the range and least table split that
+ * is after the range.
+ */
+ public static KeyExtent widen(Ample ample, KeyExtent extent) {
+ Text prevRowOfStartRowTablet = extent.prevEndRow();
+ Text endRowOfEndRowTablet = extent.endRow();
+
+ if (extent.prevEndRow() != null) {
+ // The startRow is not inclusive, so do not want to find the tablet containing startRow but
+ // instead find the tablet that contains the next possible row after startRow
+ Text nextPossibleRow = new Key(extent.prevEndRow()).followingKey(PartialKey.ROW).getRow();
+ prevRowOfStartRowTablet =
+ findContaining(ample, extent.tableId(), nextPossibleRow).prevEndRow();
+ }
+
+ if (extent.endRow() != null) {
+ // find the tablet containing endRow and use its endRow
+ endRowOfEndRowTablet = findContaining(ample, extent.tableId(), extent.endRow()).endRow();
+ }
+
+ return new KeyExtent(extent.tableId(), endRowOfEndRowTablet, prevRowOfStartRowTablet);
+ }
+
+ public static LockRange widen(Ample ample, TableId tableId, LockRange range, TableOperation op,
+ boolean tableMustExists) throws AcceptableThriftTableOperationException {
+ if (range.isInfinite()) {
+ return range;
+ }
+ try {
+ var wext = widen(ample, new KeyExtent(tableId, range.getEndRow(), range.getStartRow()));
+ return LockRange.of(wext.prevEndRow(), wext.endRow());
+ } catch (NoSuchElementException nse) {
+ if (tableMustExists) {
+ throw new AcceptableThriftTableOperationException(tableId.canonical(), "", op,
+ TableOperationExceptionType.NOTFOUND, "Table does not exist");
+ } else {
+ log.debug("Attempted to widen range {} but no metadata entries found for table {}", range,
+ tableId);
+ return LockRange.infinite();
+ }
+ }
+
+ }
+
public static long reserveTable(Manager env, TableId tableId, FateId fateId, LockType lockType,
boolean tableMustExist, TableOperation op) throws Exception {
- if (getLock(env.getContext(), tableId, fateId, lockType).tryLock()) {
+ return reserveTable(env, tableId, fateId, lockType, tableMustExist, op, LockRange.infinite());
+ }
+
+ public static long reserveTable(Manager env, TableId tableId, FateId fateId, LockType lockType,
+ boolean tableMustExist, TableOperation op, final LockRange range) throws Exception {
+ final LockRange widenedRange;
+ if (lockType == LockType.WRITE || op == TableOperation.COMPACT) {
+ /*
+ * Write locks are widened to table split points to avoid non-overlapping ranges operating on
+ * the same tablet. For example assume a table has splits at c,e and two fate operations need
+ * write locks on ranges (a1,c2] and (d1,d9]. The fate ranges do not overlap, however both
+ * will operate on the tablet (c,e]. When the ranges are widened, (a1,c2] turns into (null,e]
+ * and (d1,d9] turns into (c,e]. The widened ranges for the fate operations overlap which
+ * prevents both from operating on the same tablet.
+ *
+ * Widening is done for write locks because those need to work on mutually exclusive sets of
+ * tablets w/ other write or read locks. Read locks do not need to be mutually exclusive w/
+ * each other and it does not matter if they operate on the same tablet so widening is not
+ * needed for that case. Widening write locks is sufficient for detecting overlap w/ any
+ * tablets that read locks need to use. For example if a write locks is obtained on (a1,c2]
+ * and it widens to (null,e] then a read lock on (d1,d9] would overlap with the widened range.
+ *
+ * Mostly widening for write locks is nice because fate operations that get read locks are
+ * probably more frequent. So the work of widening is not done on most fate operations. Also
+ * widening an infinite range is a quick operation, so create/delete table will not be slowed
+ * down by widening.
+ *
+ * Widening is done for compactions because those operations widen their range.
+ */
+ widenedRange = widen(env.getContext().getAmple(), tableId, range, op, tableMustExist);
+ log.debug("{} widened write lock range from {} to {}", fateId, range, widenedRange);
+ } else {
+ widenedRange = range;
+ }
+
+ var lock = getLock(env.getContext(), tableId, fateId, lockType, widenedRange);
+ if (lockType == LockType.WRITE && !widenedRange.equals(lock.getRange())) {
+ // It is possible the range changed since the lock entry was created. Pre existing locks are
+ // found using the fate id and could have a different range.
+ lock.unlock();
+ // This should be a rare event, log at info in case it happens a lot.
+ log.info(
+ "{} widened range {} differs from existing lock range {}, deleted lock and will retry",
+ fateId, widenedRange, lock.getRange());
+ return 100;
+ } else if (lockType == LockType.READ) {
+ // Not expecting the lock range on a read lock to change.
+ Preconditions.checkState(widenedRange.equals(lock.getRange()));
+ }
+
+ if (lock.tryLock()) {
+ if (lockType == LockType.WRITE) {
+ // Now that table lock is acquired see if the range still widens to the same thing. If not
+ // it means the table splits changed so release the lock and try again later. Once the lock
+ // is acquired assuming the table splits in this range can not change, so this recheck is
+ // done after getting the lock.
Review Comment:
```suggestion
// it means the table splits changed so release the lock and try again later. The table splits in this range can not change once the lock is acquired, so this recheck is done after getting the lock.
```
just a bit more clear
##########
server/manager/src/main/java/org/apache/accumulo/manager/tableOps/Utils.java:
##########
@@ -80,17 +91,142 @@ public static <T extends AbstractId<T>> T getNextId(String name, ServerContext c
static final Lock tableNameLock = new ReentrantLock();
+ private static KeyExtent findContaining(Ample ample, TableId tableId, Text row) {
+ Objects.requireNonNull(row);
+ try (var tablets = ample.readTablets().forTable(tableId).overlapping(row, true, row)
+ .fetch(TabletMetadata.ColumnType.PREV_ROW).build()) {
+ return tablets.stream().collect(MoreCollectors.onlyElement()).getExtent();
+ }
+ }
+
+ /**
+ * Widen a range to the greatest table split that is before the range and least table split that
+ * is after the range.
+ */
+ public static KeyExtent widen(Ample ample, KeyExtent extent) {
+ Text prevRowOfStartRowTablet = extent.prevEndRow();
+ Text endRowOfEndRowTablet = extent.endRow();
+
+ if (extent.prevEndRow() != null) {
+ // The startRow is not inclusive, so do not want to find the tablet containing startRow but
+ // instead find the tablet that contains the next possible row after startRow
+ Text nextPossibleRow = new Key(extent.prevEndRow()).followingKey(PartialKey.ROW).getRow();
+ prevRowOfStartRowTablet =
+ findContaining(ample, extent.tableId(), nextPossibleRow).prevEndRow();
+ }
+
+ if (extent.endRow() != null) {
+ // find the tablet containing endRow and use its endRow
+ endRowOfEndRowTablet = findContaining(ample, extent.tableId(), extent.endRow()).endRow();
+ }
+
+ return new KeyExtent(extent.tableId(), endRowOfEndRowTablet, prevRowOfStartRowTablet);
+ }
+
+ public static LockRange widen(Ample ample, TableId tableId, LockRange range, TableOperation op,
+ boolean tableMustExists) throws AcceptableThriftTableOperationException {
+ if (range.isInfinite()) {
+ return range;
+ }
+ try {
+ var wext = widen(ample, new KeyExtent(tableId, range.getEndRow(), range.getStartRow()));
+ return LockRange.of(wext.prevEndRow(), wext.endRow());
+ } catch (NoSuchElementException nse) {
+ if (tableMustExists) {
+ throw new AcceptableThriftTableOperationException(tableId.canonical(), "", op,
+ TableOperationExceptionType.NOTFOUND, "Table does not exist");
+ } else {
+ log.debug("Attempted to widen range {} but no metadata entries found for table {}", range,
+ tableId);
+ return LockRange.infinite();
+ }
+ }
+
+ }
+
public static long reserveTable(Manager env, TableId tableId, FateId fateId, LockType lockType,
boolean tableMustExist, TableOperation op) throws Exception {
- if (getLock(env.getContext(), tableId, fateId, lockType).tryLock()) {
+ return reserveTable(env, tableId, fateId, lockType, tableMustExist, op, LockRange.infinite());
+ }
+
+ public static long reserveTable(Manager env, TableId tableId, FateId fateId, LockType lockType,
+ boolean tableMustExist, TableOperation op, final LockRange range) throws Exception {
+ final LockRange widenedRange;
+ if (lockType == LockType.WRITE || op == TableOperation.COMPACT) {
+ /*
+ * Write locks are widened to table split points to avoid non-overlapping ranges operating on
+ * the same tablet. For example assume a table has splits at c,e and two fate operations need
+ * write locks on ranges (a1,c2] and (d1,d9]. The fate ranges do not overlap, however both
+ * will operate on the tablet (c,e]. When the ranges are widened, (a1,c2] turns into (null,e]
+ * and (d1,d9] turns into (c,e]. The widened ranges for the fate operations overlap which
+ * prevents both from operating on the same tablet.
+ *
+ * Widening is done for write locks because those need to work on mutually exclusive sets of
+ * tablets w/ other write or read locks. Read locks do not need to be mutually exclusive w/
+ * each other and it does not matter if they operate on the same tablet so widening is not
+ * needed for that case. Widening write locks is sufficient for detecting overlap w/ any
+ * tablets that read locks need to use. For example if a write locks is obtained on (a1,c2]
+ * and it widens to (null,e] then a read lock on (d1,d9] would overlap with the widened range.
+ *
+ * Mostly widening for write locks is nice because fate operations that get read locks are
+ * probably more frequent. So the work of widening is not done on most fate operations. Also
+ * widening an infinite range is a quick operation, so create/delete table will not be slowed
+ * down by widening.
+ *
+ * Widening is done for compactions because those operations widen their range.
+ */
Review Comment:
Very good explanation. I wonder if there's a better/more visible spot for this. This spot seems fine though
##########
core/src/main/java/org/apache/accumulo/core/fate/AdminUtil.java:
##########
@@ -417,9 +430,10 @@ public void print(Map<FateInstanceType,ReadOnlyFateStore<T>> readOnlyFateStores,
for (TransactionStatus txStatus : fateStatus.getTransactions()) {
fmt.format(
- "%-15s fateId: %s status: %-18s locked: %-15s locking: %-15s op: %-15s created: %s%n",
+ "%-15s fateId: %s status: %-18s locked: %-15s locking: %-15s op: %-15s created: %s lock range: %s%n",
txStatus.getFateOp(), txStatus.getFateId(), txStatus.getStatus(), txStatus.getHeldLocks(),
- txStatus.getWaitingLocks(), txStatus.getTop(), txStatus.getTimeCreatedFormatted());
+ txStatus.getWaitingLocks(), txStatus.getTop(), txStatus.getTimeCreatedFormatted(),
+ txStatus.getLockRange());
Review Comment:
would be good to confirm if any of the fate command descriptions should be updated with this change.
##########
server/manager/src/main/java/org/apache/accumulo/manager/tableOps/Utils.java:
##########
@@ -180,8 +316,9 @@ public static Lock getTableNameLock() {
return tableNameLock;
}
- public static Lock getReadLock(Manager env, AbstractId<?> id, FateId fateId) {
- return Utils.getLock(env.getContext(), id, fateId, LockType.READ);
+ public static DistributedLock getReadLock(Manager env, AbstractId<?> id, FateId fateId,
+ LockRange range) {
+ return Utils.getLock(env.getContext(), id, fateId, LockType.READ, range);
Review Comment:
Do `Utils.getReadLock` and `Utils.reserveTable/Namespace(...DistributedReadWriteLock.LockType.READ...)` seem to accomplish the same thing? Can these be combined?
--
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] adds ranges to table locks [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on PR #5786:
URL: https://github.com/apache/accumulo/pull/5786#issuecomment-3168736893
This change is important for the new automatic merge feature. Even though merges are fast now, if a merge gets stuck waiting on a long running compaction then it will block all subsequent compactions and bulk imports from running. Adding ranges prevents blocking for the case where the ranges do not overlap.
This PR can also close #1324 #3823
--
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] adds ranges to table locks [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on PR #5786:
URL: https://github.com/apache/accumulo/pull/5786#issuecomment-3229505804
> I didn't follow all of the bulk import changes
The cause of the bulk changes was the code was computing the range after it got the lock. Needed to reorg the code to compute the range prior to getting the lock.
--
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] adds ranges to table locks [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #5786:
URL: https://github.com/apache/accumulo/pull/5786#discussion_r2305238257
##########
server/manager/src/main/java/org/apache/accumulo/manager/tableOps/Utils.java:
##########
@@ -180,8 +316,9 @@ public static Lock getTableNameLock() {
return tableNameLock;
}
- public static Lock getReadLock(Manager env, AbstractId<?> id, FateId fateId) {
- return Utils.getLock(env.getContext(), id, fateId, LockType.READ);
+ public static DistributedLock getReadLock(Manager env, AbstractId<?> id, FateId fateId,
+ LockRange range) {
+ return Utils.getLock(env.getContext(), id, fateId, LockType.READ, range);
Review Comment:
The can be used to get the lock if it exists w/o creating it in ZK if it does not.
--
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] adds ranges to table locks [accumulo]
Posted by "kevinrr888 (via GitHub)" <gi...@apache.org>.
kevinrr888 commented on code in PR #5786:
URL: https://github.com/apache/accumulo/pull/5786#discussion_r2286164039
##########
server/manager/src/main/java/org/apache/accumulo/manager/tableOps/bulkVer2/ComputeBulkRange.java:
##########
@@ -0,0 +1,75 @@
+/*
+ * 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.manager.tableOps.bulkVer2;
+
+import java.util.Optional;
+
+import org.apache.accumulo.core.clientImpl.bulk.BulkSerialize;
+import org.apache.accumulo.core.clientImpl.bulk.LoadMappingIterator;
+import org.apache.accumulo.core.data.TableId;
+import org.apache.accumulo.core.dataImpl.KeyExtent;
+import org.apache.accumulo.core.fate.FateId;
+import org.apache.accumulo.core.fate.Repo;
+import org.apache.accumulo.manager.Manager;
+import org.apache.accumulo.manager.tableOps.ManagerRepo;
+import org.apache.accumulo.server.fs.VolumeManager;
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.io.Text;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class ComputeBulkRange extends ManagerRepo {
+ private static final long serialVersionUID = 1L;
+
+ private static final Logger log = LoggerFactory.getLogger(ComputeBulkRange.class);
+
+ private final BulkInfo bulkInfo;
+
+ public ComputeBulkRange(TableId tableId, String sourceDir, boolean setTime) {
+ BulkInfo info = new BulkInfo();
+ info.tableId = tableId;
+ info.sourceDir = sourceDir;
+ info.setTime = setTime;
+ this.bulkInfo = info;
Review Comment:
ah I see now, I read all the `info.` as `this.` out of habit
--
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] adds ranges to table locks [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #5786:
URL: https://github.com/apache/accumulo/pull/5786#discussion_r2286086811
##########
server/manager/src/main/java/org/apache/accumulo/manager/tableOps/bulkVer2/ComputeBulkRange.java:
##########
@@ -0,0 +1,75 @@
+/*
+ * 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.manager.tableOps.bulkVer2;
+
+import java.util.Optional;
+
+import org.apache.accumulo.core.clientImpl.bulk.BulkSerialize;
+import org.apache.accumulo.core.clientImpl.bulk.LoadMappingIterator;
+import org.apache.accumulo.core.data.TableId;
+import org.apache.accumulo.core.dataImpl.KeyExtent;
+import org.apache.accumulo.core.fate.FateId;
+import org.apache.accumulo.core.fate.Repo;
+import org.apache.accumulo.manager.Manager;
+import org.apache.accumulo.manager.tableOps.ManagerRepo;
+import org.apache.accumulo.server.fs.VolumeManager;
+import org.apache.hadoop.fs.Path;
+import org.apache.hadoop.io.Text;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class ComputeBulkRange extends ManagerRepo {
+ private static final long serialVersionUID = 1L;
+
+ private static final Logger log = LoggerFactory.getLogger(ComputeBulkRange.class);
+
+ private final BulkInfo bulkInfo;
+
+ public ComputeBulkRange(TableId tableId, String sourceDir, boolean setTime) {
+ BulkInfo info = new BulkInfo();
+ info.tableId = tableId;
+ info.sourceDir = sourceDir;
+ info.setTime = setTime;
+ this.bulkInfo = info;
Review Comment:
This suggestion does not work because `info` is not defined.
--
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] adds ranges to table locks [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #5786:
URL: https://github.com/apache/accumulo/pull/5786#discussion_r2286190413
##########
core/src/test/java/org/apache/accumulo/core/fate/zookeeper/DistributedReadWriteLockTest.java:
##########
@@ -146,10 +149,69 @@ public void testLock() throws Exception {
}
}
+ private DistributedLock recover(QueueLock queueLock, FateId fateId, LockRange expectedRange,
+ LockType expectedType) {
+ var dlock = DistributedReadWriteLock.recoverLock(queueLock, fateId);
+ assertEquals(expectedRange, dlock.getRange());
+ assertEquals(expectedType, dlock.getType());
+ return dlock;
+ }
+
+ @Test
+ public void testRanges() {
Review Comment:
added a test in ffdc5fb1a806c55ed1c2c3c738ecfeaae5116e35
--
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] adds ranges to table locks [accumulo]
Posted by "DomGarguilo (via GitHub)" <gi...@apache.org>.
DomGarguilo commented on code in PR #5786:
URL: https://github.com/apache/accumulo/pull/5786#discussion_r2305057151
##########
core/src/main/java/org/apache/accumulo/core/fate/zookeeper/FateLock.java:
##########
@@ -251,7 +264,7 @@ public void removeEntry(FateLockEntry data, long entry) {
public static SortedSet<NodeName> validateAndWarn(FateLockPath path, List<String> children) {
log.trace("validating and sorting children at path {}", path);
- SortedSet<NodeName> validChildren = new TreeSet<>();
+ SortedSet<NodeName> validChildren = new TreeSet<>(Comparator.comparingLong(nn -> nn.sequence));
Review Comment:
Should we refactor this comparator into a constant for reuse?
##########
core/src/main/java/org/apache/accumulo/core/fate/AdminUtil.java:
##########
@@ -282,36 +293,42 @@ private void findLocks(ZooSession zk, final ServiceLockPath lockPath,
SortedSet<FateLock.NodeName> lockNodes =
FateLock.validateAndWarn(fLockPath, zr.getChildren(fLockPath.toString()));
- int pos = 0;
- boolean sawWriteLock = false;
+ ArrayList<FateLock.FateLockEntry> previous = new ArrayList<>(lockNodes.size());
for (FateLock.NodeName node : lockNodes) {
try {
FateLock.FateLockEntry fateLockEntry = node.fateLockEntry.get();
var fateId = fateLockEntry.getFateId();
var lockType = fateLockEntry.getLockType();
- if (lockType == LockType.WRITE) {
- sawWriteLock = true;
- }
+ lockRanges.put(fateId, fateLockEntry.getRange());
- Map<FateId,List<String>> locks;
+ Map<FateId,List<String>> locks = heldLocks;
- if (pos == 0) {
- locks = heldLocks;
- } else if (lockType == LockType.READ && !sawWriteLock) {
- locks = heldLocks;
+ if (lockType == LockType.WRITE) {
+ for (var prev : Lists.reverse(previous)) {
Review Comment:
`Lists.reverse()` might add a small amount of overhead. Its definitely more simple and readable but indexed iteration might be slightly more performant.
--
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] adds ranges to table locks [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner merged PR #5786:
URL: https://github.com/apache/accumulo/pull/5786
--
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] adds ranges to table locks [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #5786:
URL: https://github.com/apache/accumulo/pull/5786#discussion_r2286135036
##########
core/src/main/java/org/apache/accumulo/core/fate/zookeeper/FateLock.java:
##########
@@ -154,26 +176,14 @@ public static class NodeName implements Comparable<NodeName> {
.memoize(() -> FateLockEntry.deserialize(nodeName.substring(PREFIX.length(), len - 11)));
}
- @Override
- public int compareTo(NodeName o) {
- int cmp = Long.compare(sequence, o.sequence);
- if (cmp == 0) {
- cmp = fateLockEntry.get().compareTo(o.fateLockEntry.get());
- }
- return cmp;
- }
-
@Override
public boolean equals(Object o) {
- if (o instanceof NodeName) {
- return this.compareTo((NodeName) o) == 0;
- }
- return false;
+ throw new UnsupportedOperationException();
}
@Override
public int hashCode() {
- return Objects.hash(sequence, fateLockEntry.get());
+ throw new UnsupportedOperationException();
Review Comment:
I suspect these are not used. I added new fields to the class and rather than add complexity to an untested and unused method I made them throw an exception. It is hard to tell if these are actually used, do not have good luck looking for what uses these methods in my IDE. So if something is using them it will eventually throw an exception.
Not sure, but I think these method were initially implemented because the class had a compareTo method and a build plugin required these methods also.
I will leave this comment open as a reminder to remove these methods after running more test.
--
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] adds ranges to table locks [accumulo]
Posted by "kevinrr888 (via GitHub)" <gi...@apache.org>.
kevinrr888 commented on code in PR #5786:
URL: https://github.com/apache/accumulo/pull/5786#discussion_r2304623541
##########
server/manager/src/main/java/org/apache/accumulo/manager/tableOps/Utils.java:
##########
@@ -180,8 +316,9 @@ public static Lock getTableNameLock() {
return tableNameLock;
}
- public static Lock getReadLock(Manager env, AbstractId<?> id, FateId fateId) {
- return Utils.getLock(env.getContext(), id, fateId, LockType.READ);
+ public static DistributedLock getReadLock(Manager env, AbstractId<?> id, FateId fateId,
+ LockRange range) {
+ return Utils.getLock(env.getContext(), id, fateId, LockType.READ, range);
Review Comment:
Do `Utils.getReadLock` and `Utils.reserveTable/Namespace(...DistributedReadWriteLock.LockType.READ...)` accomplish the same thing? Can these be combined?
--
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] adds ranges to table locks [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #5786:
URL: https://github.com/apache/accumulo/pull/5786#discussion_r2286120182
##########
core/src/main/java/org/apache/accumulo/core/fate/zookeeper/FateLock.java:
##########
@@ -251,7 +261,7 @@ public void removeEntry(FateLockEntry data, long entry) {
public static SortedSet<NodeName> validateAndWarn(FateLockPath path, List<String> children) {
log.trace("validating and sorting children at path {}", path);
- SortedSet<NodeName> validChildren = new TreeSet<>();
+ SortedSet<NodeName> validChildren = new TreeSet<>(Comparator.comparingLong(nn -> nn.sequence));
Review Comment:
This is the only code that needs to compare NodeName objs and the code only cares about the sequence. Adding compareTo method to the class should compare on all fields in the class, which adds a lot of complexity to the code that is not needed. So just went w/ a simpler comparator in the place where its needed.
--
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] adds ranges to table locks [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #5786:
URL: https://github.com/apache/accumulo/pull/5786#discussion_r2286137288
##########
core/src/main/java/org/apache/accumulo/core/fate/zookeeper/FateLock.java:
##########
@@ -68,19 +72,22 @@ public String toString() {
}
}
- public static class FateLockEntry implements Comparable<FateLockEntry> {
+ public static class FateLockEntry {
final LockType lockType;
final FateId fateId;
+ final LockRange range;
- private FateLockEntry(LockType lockType, FateId fateId) {
+ private FateLockEntry(LockType lockType, FateId fateId, LockRange range) {
this.lockType = Objects.requireNonNull(lockType);
this.fateId = Objects.requireNonNull(fateId);
+ this.range = range;
}
private FateLockEntry(String entry) {
- var fields = entry.split(":", 2);
+ var fields = entry.split("_", 4);
Review Comment:
updated in 2bcfc43ab4fc5770025b631047a89a1eb8dc464b
--
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] adds ranges to table locks [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on PR #5786:
URL: https://github.com/apache/accumulo/pull/5786#issuecomment-3229490836
I was running bulk random walk test against this branch and found #5833. The test is a good test for this change because it does lots of concurrent table operations with different ranges.
--
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] adds ranges to table locks [accumulo]
Posted by "keith-turner (via GitHub)" <gi...@apache.org>.
keith-turner commented on code in PR #5786:
URL: https://github.com/apache/accumulo/pull/5786#discussion_r2305180617
##########
core/src/main/java/org/apache/accumulo/core/fate/AdminUtil.java:
##########
@@ -282,36 +293,42 @@ private void findLocks(ZooSession zk, final ServiceLockPath lockPath,
SortedSet<FateLock.NodeName> lockNodes =
FateLock.validateAndWarn(fLockPath, zr.getChildren(fLockPath.toString()));
- int pos = 0;
- boolean sawWriteLock = false;
+ ArrayList<FateLock.FateLockEntry> previous = new ArrayList<>(lockNodes.size());
for (FateLock.NodeName node : lockNodes) {
try {
FateLock.FateLockEntry fateLockEntry = node.fateLockEntry.get();
var fateId = fateLockEntry.getFateId();
var lockType = fateLockEntry.getLockType();
- if (lockType == LockType.WRITE) {
- sawWriteLock = true;
- }
+ lockRanges.put(fateId, fateLockEntry.getRange());
- Map<FateId,List<String>> locks;
+ Map<FateId,List<String>> locks = heldLocks;
- if (pos == 0) {
- locks = heldLocks;
- } else if (lockType == LockType.READ && !sawWriteLock) {
- locks = heldLocks;
+ if (lockType == LockType.WRITE) {
+ for (var prev : Lists.reverse(previous)) {
Review Comment:
I think this code is ok because its not called too frequently.
--
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