You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2020/09/29 03:54:13 UTC
[GitHub] [hadoop-ozone] bharatviswa504 opened a new pull request #1451: Hdds 4117
bharatviswa504 opened a new pull request #1451:
URL: https://github.com/apache/hadoop-ozone/pull/1451
## What changes were proposed in this pull request?
Normalize Keypath for listKeys.
When ozone.om.enable.filesystem.paths, OM normalizes path, and stores the Keyname in the OM DB KeyTable.
When listKeys uses given keyName(not normalized key path) as prefix and Starkey the list-keys will return an empty results.
Similar to HDDS-4102, we should normalize Starkey and keyPrefix.
## What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-4117
## How was this patch tested?
Added a test.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] elek commented on a change in pull request #1451: HDDS-4117. Normalize Keypath for listKeys.
Posted by GitBox <gi...@apache.org>.
elek commented on a change in pull request #1451:
URL: https://github.com/apache/hadoop-ozone/pull/1451#discussion_r502357778
##########
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.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] bharatviswa504 commented on pull request #1451: HDDS-4117. Normalize Keypath for listKeys.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on pull request #1451:
URL: https://github.com/apache/hadoop-ozone/pull/1451#issuecomment-700951976
Rebased and fixed test.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] elek commented on a change in pull request #1451: HDDS-4117. Normalize Keypath for listKeys.
Posted by GitBox <gi...@apache.org>.
elek commented on a change in pull request #1451:
URL: https://github.com/apache/hadoop-ozone/pull/1451#discussion_r502356956
##########
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)
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] bharatviswa504 commented on a change in pull request #1451: HDDS-4117. Normalize Keypath for listKeys.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #1451:
URL: https://github.com/apache/hadoop-ozone/pull/1451#discussion_r508891177
##########
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 for clear explanation, I have updated to move null check to normalizeKey method.
>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.
Updated OmUtils.normalizeKey to take the boolean flag to preseverTralingSlash if true will preserve it. In this way, this method can be used in listKey case with true, rest all code with false. Have a look in to it, and share ur suggestions on the approach.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] bharatviswa504 commented on a change in pull request #1451: HDDS-4117. Normalize Keypath for listKeys.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #1451:
URL: https://github.com/apache/hadoop-ozone/pull/1451#discussion_r499806149
##########
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:
As anyway this condition needs to be checked inside normalizeListKeyPath, so not performed check again 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.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] bharatviswa504 commented on a change in pull request #1451: HDDS-4117. Normalize Keypath for listKeys.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #1451:
URL: https://github.com/apache/hadoop-ozone/pull/1451#discussion_r508891177
##########
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 for clear explanation, I have updated to move null check to normalizeKey method.
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.
Updated OmUtils.normalizeKey to take the boolean flag to preseverTralingSlash if true will preserve it. In this way, this method can be used in listKey case with true, rest all code with false. Have a look in to it, and share ur suggestions on the approach.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] elek commented on a change in pull request #1451: HDDS-4117. Normalize Keypath for listKeys.
Posted by GitBox <gi...@apache.org>.
elek commented on a change in pull request #1451:
URL: https://github.com/apache/hadoop-ozone/pull/1451#discussion_r499525922
##########
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:
NIT:
I think it's easier to follow to move the condition to here:
```
if (enableFileSystemPaths) {
startKey = normalizeListKeyPath(startKey);
keyPrefix = normalizeListKeyPath(keyPrefix);
}
```
Despite the name, the `startKey` and `keyPrefix` are not normalized if `enableFileSystemPaths` is false
##########
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:
Do we need this method at all? Is it possible to have invalid key path after `OmUtils.normalizeKey`? Seems to be an unnecessary additional step.
##########
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:
Is there any reason to keep these two features (return empty string for empty string + keep closing `/`) in here intead of moving to `OmUtils.normalizeKey(keyPath)`
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] elek commented on a change in pull request #1451: HDDS-4117. Normalize Keypath for listKeys.
Posted by GitBox <gi...@apache.org>.
elek commented on a change in pull request #1451:
URL: https://github.com/apache/hadoop-ozone/pull/1451#discussion_r502358863
##########
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] bharatviswa504 commented on a change in pull request #1451: HDDS-4117. Normalize Keypath for listKeys.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #1451:
URL: https://github.com/apache/hadoop-ozone/pull/1451#discussion_r499800917
##########
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:
Yes, there are a few cases.
For examples refer testNormalizeKeyInvalidPaths test in TestNormalizePaths.java
https://github.com/apache/hadoop-ozone/blob/master/hadoop-ozone/ozone-manager/src/test/java/org/apache/hadoop/ozone/om/request/TestNormalizePaths.java#L71
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] elek commented on a change in pull request #1451: HDDS-4117. Normalize Keypath for listKeys.
Posted by GitBox <gi...@apache.org>.
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] elek commented on a change in pull request #1451: HDDS-4117. Normalize Keypath for listKeys.
Posted by GitBox <gi...@apache.org>.
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.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] bharatviswa504 commented on a change in pull request #1451: HDDS-4117. Normalize Keypath for listKeys.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #1451:
URL: https://github.com/apache/hadoop-ozone/pull/1451#discussion_r508888638
##########
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:
Thanks to explain it. I understood your point. Updated the code.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] bharatviswa504 commented on pull request #1451: HDDS-4117. Normalize Keypath for listKeys.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on pull request #1451:
URL: https://github.com/apache/hadoop-ozone/pull/1451#issuecomment-713190031
Thank You @elek for the review.
I have addressed the review comments.
----------------------------------------------------------------
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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org
[GitHub] [hadoop-ozone] bharatviswa504 commented on a change in pull request #1451: HDDS-4117. Normalize Keypath for listKeys.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #1451:
URL: https://github.com/apache/hadoop-ozone/pull/1451#discussion_r499805554
##########
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:
For key ending with "/" after normalizing it will return the key without trailing "/". As here the key intended to be searched is, I have added it back here. In my view removing "/" at the end when the user explicitly asked to search for keys end with "/", it might return results differently, as I don't want to change current semantics.
Example:
the prefix is "/a/b/c/d/" -> after Normalize "/a/b/c/d"
So if we don't add "/" back, it will return keys with prefix "/a/b/c/d", but user intention here is to get keys in "/a/b/c/d/" so not to change semantics for this case handled in this method instead of common method used by all classes.
And also for null not calling OmUtils.normalizeKey(keyPath), the Paths method will fail with NPE, so this is also considered a special case and handled it in this 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:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: ozone-issues-unsubscribe@hadoop.apache.org
For additional commands, e-mail: ozone-issues-help@hadoop.apache.org