You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@accumulo.apache.org by GitBox <gi...@apache.org> on 2022/10/16 15:17:26 UTC
[GitHub] [accumulo] cshannon commented on a diff in pull request #3003: remove extra node in prop path
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: notifications-unsubscribe@accumulo.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org