ctubbsii commented on code in PR #5417:
URL: https://github.com/apache/accumulo/pull/5417#discussion_r2002145459
##########
core/src/main/java/org/apache/accumulo/core/clientImpl/ConditionalWriterImpl.java:
##########
@@ -683,7 +682,7 @@ private void invalidateSession(SessionID sessionId,
HostAndPort location)
long startTime = System.currentTimeMillis();
- LockID lid = new LockID(Constants.ZTSERVERS, sessionId.lockId);
+ LockID lid = LockID.deserialize(sessionId.lockId);
Review Comment:
Do you know why this one had the `/tservers` part in there? I couldn't
figure out why that was there, even before the chroot changes.
##########
core/src/main/java/org/apache/accumulo/core/fate/zookeeper/ZooUtil.java:
##########
@@ -67,33 +71,48 @@ public static class LockID {
public final long eid;
public final String path;
public final String node;
+ private final Supplier<String> serialized;
+
+ public LockID(String path, String node, long eid) {
+ // Do not allow the path to end with / because this would cause problems
for serialization. Do
+ // not allow $ char in path as this would cause problems for
serialization. Current code does
+ // not pass these as input. The path is expected to be an absolute path,
so check it starts
+ // with /.
+ Preconditions.checkArgument(
+ path != null && !path.contains("$") && path.startsWith("/") &&
!path.endsWith("/"),
+ "Illegal path %s", path);
+ // Do not allow $ and / chars in the node name as this would cause
problems for serialization.
+ // Current code does not pass these as input.
Review Comment:
```suggestion
// node must not contain '$' or '/'
```
##########
core/src/main/java/org/apache/accumulo/core/fate/zookeeper/ZooUtil.java:
##########
@@ -67,33 +71,48 @@ public static class LockID {
public final long eid;
public final String path;
public final String node;
+ private final Supplier<String> serialized;
+
+ public LockID(String path, String node, long eid) {
+ // Do not allow the path to end with / because this would cause problems
for serialization. Do
+ // not allow $ char in path as this would cause problems for
serialization. Current code does
+ // not pass these as input. The path is expected to be an absolute path,
so check it starts
+ // with /.
Review Comment:
The constraint on the path could be made more succinct:
```suggestion
// path must start with a '/', must not end with one, and must not
contain '$'.
```
##########
core/src/main/java/org/apache/accumulo/core/fate/zookeeper/ZooUtil.java:
##########
@@ -67,33 +71,48 @@ public static class LockID {
public final long eid;
public final String path;
public final String node;
+ private final Supplier<String> serialized;
+
+ public LockID(String path, String node, long eid) {
+ // Do not allow the path to end with / because this would cause problems
for serialization. Do
+ // not allow $ char in path as this would cause problems for
serialization. Current code does
+ // not pass these as input. The path is expected to be an absolute path,
so check it starts
+ // with /.
+ Preconditions.checkArgument(
+ path != null && !path.contains("$") && path.startsWith("/") &&
!path.endsWith("/"),
+ "Illegal path %s", path);
Review Comment:
I did not know you could use format strings with Preconditions!
--
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]
