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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
