cshannon commented on code in PR #5262:
URL: https://github.com/apache/accumulo/pull/5262#discussion_r1938321556
##########
core/src/main/java/org/apache/accumulo/core/fate/zookeeper/FateLock.java:
##########
@@ -76,26 +77,10 @@ private FateLockEntry(LockType lockType, FateId fateId) {
this.fateId = Objects.requireNonNull(fateId);
}
- private FateLockEntry(byte[] entry) {
- if (entry == null || entry.length < 1) {
- throw new IllegalArgumentException();
- }
-
- int split = -1;
- for (int i = 0; i < entry.length; i++) {
- if (entry[i] == ':') {
- split = i;
- break;
- }
- }
-
- if (split == -1) {
- throw new IllegalArgumentException();
- }
-
- this.lockType = LockType.valueOf(new String(entry, 0, split, UTF_8));
- this.fateId =
- FateId.from(new String(Arrays.copyOfRange(entry, split + 1,
entry.length), UTF_8));
+ private FateLockEntry(String entry) {
+ var fields = entry.split(":", 2);
Review Comment:
Any reason to verify the number of fields is 2? I suppose it will just fail
a couple lines later if not correct.
##########
core/src/main/java/org/apache/accumulo/core/fate/zookeeper/FateLock.java:
##########
@@ -106,14 +91,8 @@ public FateId getFateId() {
return fateId;
}
- public byte[] serialize() {
- byte[] typeBytes = lockType.name().getBytes(UTF_8);
- byte[] fateIdBytes = fateId.canonical().getBytes(UTF_8);
- byte[] result = new byte[fateIdBytes.length + 1 + typeBytes.length];
- System.arraycopy(typeBytes, 0, result, 0, typeBytes.length);
- result[typeBytes.length] = ':';
- System.arraycopy(fateIdBytes, 0, result, typeBytes.length + 1,
fateIdBytes.length);
- return result;
+ public String serialize() {
+ return lockType.name() + ":" + fateId.canonical();
Review Comment:
This is SO much nicer, not having to do all that manual effort to copy bytes
around is great.
##########
core/src/main/java/org/apache/accumulo/core/fate/zookeeper/FateLock.java:
##########
@@ -184,17 +214,11 @@ public SortedMap<Long,Supplier<FateLockEntry>>
getEarlierEntries(long entry) {
}
for (String name : children) {
- // this try catch must be done inside the loop because some subset of
the children may exist
- try {
- long order = Long.parseLong(name.substring(PREFIX.length()));
- if (order <= entry) {
- byte[] data = zoo.getData(path + "/" + name);
- // Use a supplier so we don't need to deserialize unless the
calling code cares about
- // the value for that entry.
- result.put(order, Suppliers.memoize(() ->
FateLockEntry.deserialize(data)));
- }
- } catch (KeeperException.NoNodeException ex) {
- // 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
Review Comment:
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
--
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]
