cshannon commented on code in PR #3003:
URL: https://github.com/apache/accumulo/pull/3003#discussion_r996450653
##########
server/base/src/main/java/org/apache/accumulo/server/conf/store/impl/ZooPropLoader.java:
##########
@@ -63,6 +63,9 @@ public ZooPropLoader(final ZooReaderWriter zrw, final
VersionedPropCodec propCod
Stat stat = new Stat();
byte[] bytes = zrw.getData(propStoreKey.getPath(), propStoreWatcher,
stat);
+ if (bytes.length == 0) {
Review Comment:
Can bytes be null here? I'm assuming not as I think you would just get a
NoNodeException which is handled but thought I'd bring it up.
##########
server/base/src/main/java/org/apache/accumulo/server/conf/store/PropStoreKey.java:
##########
@@ -90,24 +85,33 @@ protected PropStoreKey(final InstanceId instanceId, final
String path, final ID_
public static @Nullable PropStoreKey<?> fromPath(final String path) {
String[] tokens = path.split("/");
+ if (tokens.length < 1) {
+ return null;
+ }
+
InstanceId instanceId;
try {
instanceId = InstanceId.of(tokens[IID_TOKEN_POSITION]);
} catch (ArrayIndexOutOfBoundsException ex) {
log.warn("Path '{}' is an invalid path for a property cache key", path);
return null;
}
- if (tokens.length < 1 || !tokens[tokens.length -
1].equals(PROP_NODE_NAME)) {
- // without tokens or it does not end with PROP_NAME_NAME
- return null;
- }
- if (tokens[TYPE_TOKEN_POSITION].equals(TABLES_NODE_NAME)) {
+
+ String nodeName = "/" + tokens[tokens.length - 1];
+ if (tokens[TYPE_TOKEN_POSITION].equals(TABLES_NODE_NAME) &&
nodeName.equals(ZCONFIG)) {
return TablePropKey.of(instanceId,
TableId.of(tokens[ID_TOKEN_POSITION]));
}
- if (tokens[TYPE_TOKEN_POSITION].equals(NAMESPACE_NODE_NAME)) {
+
+ if (tokens[TYPE_TOKEN_POSITION].equals(NAMESPACE_NODE_NAME) &&
nodeName.equals(ZCONFIG)) {
return NamespacePropKey.of(instanceId,
NamespaceId.of(tokens[ID_TOKEN_POSITION]));
}
- return SystemPropKey.of(instanceId);
+
+ if (nodeName.equals(ZCONFIG)) {
Review Comment:
I was wondering if it make sense to do this check first? Since it's only one
condition in theory it should be a little faster if it's a system prop to
process as it is only 1 string comparison vs 2 for tables/namespaces. In
practice it probably won't make a noticeable difference.
##########
server/base/src/main/java/org/apache/accumulo/server/conf/store/PropStoreKey.java:
##########
@@ -90,24 +85,33 @@ protected PropStoreKey(final InstanceId instanceId, final
String path, final ID_
public static @Nullable PropStoreKey<?> fromPath(final String path) {
String[] tokens = path.split("/");
+ if (tokens.length < 1) {
+ return null;
+ }
+
InstanceId instanceId;
try {
instanceId = InstanceId.of(tokens[IID_TOKEN_POSITION]);
} catch (ArrayIndexOutOfBoundsException ex) {
log.warn("Path '{}' is an invalid path for a property cache key", path);
return null;
}
- if (tokens.length < 1 || !tokens[tokens.length -
1].equals(PROP_NODE_NAME)) {
- // without tokens or it does not end with PROP_NAME_NAME
- return null;
- }
- if (tokens[TYPE_TOKEN_POSITION].equals(TABLES_NODE_NAME)) {
+
+ String nodeName = "/" + tokens[tokens.length - 1];
+ if (tokens[TYPE_TOKEN_POSITION].equals(TABLES_NODE_NAME) &&
nodeName.equals(ZCONFIG)) {
Review Comment:
I think that in general this method could use a bit more robust validation
of the tokens length before indexing. For example, on this line the code
doesn't make sure the tokens length is long enough before indexing
TYPE_TOKEN_POSITION. Because of this if you do something like call
PropStoreKey.fromPath("/accumulo/" + iid); then an
ArrayIndexoutOfBoundsException is thrown. In theory you wouldn't normally call
this but good to handle all error cases.
One thing that might make it easier is to just wrap all the if statements
here inside the try/catch block that already exists to handle
IID_TOKEN_POSITION on line 93. That way if at any part of the processing we get
an ArrayIndexoutOfBoundsException it can be handled the same way with the
warning logged and null returned. Something like this could work:
```
try {
InstanceId instanceId = InstanceId.of(tokens[IID_TOKEN_POSITION]);
String nodeName = "/" + tokens[tokens.length - 1];
if (tokens[TYPE_TOKEN_POSITION].equals(TABLES_NODE_NAME) &&
nodeName.equals(ZCONFIG)) {
return TablePropKey.of(instanceId,
TableId.of(tokens[ID_TOKEN_POSITION]));
}
if (tokens[TYPE_TOKEN_POSITION].equals(NAMESPACE_NODE_NAME) &&
nodeName.equals(ZCONFIG)) {
return NamespacePropKey.of(instanceId,
NamespaceId.of(tokens[ID_TOKEN_POSITION]));
}
if (nodeName.equals(ZCONFIG)) {
return SystemPropKey.of(instanceId);
}
} catch (ArrayIndexOutOfBoundsException ex) {
log.warn("Path '{}' is an invalid path for a property cache key",
path);
return null;
}
```
##########
server/base/src/main/java/org/apache/accumulo/server/conf/store/impl/ZooPropStore.java:
##########
@@ -222,6 +223,10 @@ public void create(PropStoreKey<?> propStoreKey,
Map<String,String> props) {
try {
Stat stat = new Stat();
byte[] bytes = zooReader.getData(propStoreKey.getPath(), watcher, stat);
+ if (bytes.length == 0) {
Review Comment:
Same comment/question about bytes being null as above.
##########
server/base/src/main/java/org/apache/accumulo/server/conf/util/ConfigTransformer.java:
##########
@@ -152,18 +152,15 @@ VersionedProperties transform(final PropStoreKey<?>
propStoreKey, final Transfor
}
}
- Set<LegacyPropNode> upgradeNodes = readLegacyProps(propStoreKey);
- if (upgradeNodes == null) {
- log.info("Found existing node after reading legacy props {}, skipping
conversion",
+ Set<LegacyPropNode> upgradeNodes = readLegacyProps(legacyPath);
+ if (upgradeNodes.size() == 0) {
+ log.trace("No existing legacy props {}, skipping conversion, writing
default prop node",
propStoreKey);
- results = ZooPropStore.readFromZk(propStoreKey, propStoreWatcher, zrw);
- if (results != null) {
- return results;
- }
+ return writeNode(propStoreKey, Map.of());
}
upgradeNodes = convertDeprecatedProps(propStoreKey, upgradeNodes);
-
+ // todo - here
Review Comment:
Did you mean to leave this here?
--
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]
