elek commented on a change in pull request #1451:
URL: https://github.com/apache/hadoop-ozone/pull/1451#discussion_r502352349
##########
File path:
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java
##########
@@ -919,12 +920,32 @@ private boolean isKeyEmpty(OmKeyInfo keyInfo) {
// underlying table using an iterator. That automatically creates a
// snapshot of the data, so we don't need these locks at a higher level
// when we iterate.
+
+ startKey = normalizeListKeyPath(startKey);
+ keyPrefix = normalizeListKeyPath(keyPrefix);
Review comment:
Sorry, I don't get it. Why do we need to check it _inside
normalizeListKeyPath_.
As I wrote it's a very minor thing, but it seems to be better to move out
the check from the method because it improves the readability (IMHO!).
When somebody read the `listKeys` method it suggest the the keys are
normalized. but in fact it's normalized only if enableFileSystemPaths. This can
be confusing as the method name is `normalizeListKeyPath` and not something
like `normalizeListKeyPathIfNormalizationIsEnabled`.
I suggested moving out this condition from this method to improve the
readibility, but if you think it's a bad idea, it can be ignored.
##########
File path:
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/request/OMClientRequest.java
##########
@@ -298,21 +296,11 @@ public static String validateAndNormalizeKey(boolean
enableFileSystemPaths,
}
}
- @SuppressFBWarnings("DMI_HARDCODED_ABSOLUTE_FILENAME")
+
public static String validateAndNormalizeKey(String keyName)
throws OMException {
- String normalizedKeyName;
- if (keyName.startsWith(OM_KEY_PREFIX)) {
- normalizedKeyName = Paths.get(keyName).toUri().normalize().getPath();
- } else {
- normalizedKeyName = Paths.get(OM_KEY_PREFIX, keyName).toUri()
- .normalize().getPath();
- }
- if (!keyName.equals(normalizedKeyName)) {
- LOG.debug("Normalized key {} to {} ", keyName,
- normalizedKeyName.substring(1));
- }
- return isValidKeyPath(normalizedKeyName.substring(1));
+ String normalizedKeyName = OmUtils.normalizeKey(keyName);
+ return isValidKeyPath(normalizedKeyName);
Review comment:
Got it, thanks a lot. These are the cases where the normalization will
result an invalid path due to the too many `..` (for example)
##########
File path:
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java
##########
@@ -919,12 +920,32 @@ private boolean isKeyEmpty(OmKeyInfo keyInfo) {
// underlying table using an iterator. That automatically creates a
// snapshot of the data, so we don't need these locks at a higher level
// when we iterate.
+
+ startKey = normalizeListKeyPath(startKey);
+ keyPrefix = normalizeListKeyPath(keyPrefix);
+
List<OmKeyInfo> keyList = metadataManager.listKeys(volumeName, bucketName,
startKey, keyPrefix, maxKeys);
refreshPipeline(keyList);
return keyList;
}
+ private String normalizeListKeyPath(String keyPath) {
+
+ String normalizeKeyPath = keyPath;
+ if (enableFileSystemPaths) {
+ // For empty strings do nothing.
+ if (StringUtils.isBlank(keyPath)) {
Review comment:
Thanks to explain it.
> the Paths method will fail with NPE, so this is also
Not a big deal, but if we move the empty check, to `normalizeKey`, the
method will be safe forever, and we don't need to do the empty check all the
time when we need to call it.
##########
File path:
hadoop-ozone/ozone-manager/src/main/java/org/apache/hadoop/ozone/om/KeyManagerImpl.java
##########
@@ -919,12 +920,32 @@ private boolean isKeyEmpty(OmKeyInfo keyInfo) {
// underlying table using an iterator. That automatically creates a
// snapshot of the data, so we don't need these locks at a higher level
// when we iterate.
+
+ startKey = normalizeListKeyPath(startKey);
+ keyPrefix = normalizeListKeyPath(keyPrefix);
+
List<OmKeyInfo> keyList = metadataManager.listKeys(volumeName, bucketName,
startKey, keyPrefix, maxKeys);
refreshPipeline(keyList);
return keyList;
}
+ private String normalizeListKeyPath(String keyPath) {
+
+ String normalizeKeyPath = keyPath;
+ if (enableFileSystemPaths) {
+ // For empty strings do nothing.
+ if (StringUtils.isBlank(keyPath)) {
Review comment:
BTW (just a discussion not a code review comment): it can be useful to
differentiate between normalization (removing/resolving `..`, `//`) and
handling closing `/`. My impression is that we need different rules for them.
This is a good example here: you need the first ("normalization") but not
the second. (Maybe two different method?)
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscr...@hadoop.apache.org
For additional commands, e-mail: ozone-issues-h...@hadoop.apache.org
