HDDS-4117. Normalize Keypath for listKeys.

[bharatviswa504]

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.

[bharatviswa504]

Rebased and fixed test.

[elek]

Thanks @bharatviswa504

Overall it looks good to me. It turns out the key normalization only of enableFileSystemPaths is turned on.

I have a few questions inline about the implementation (seems to be possible to make it more simple)

[elek]

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

[bharatviswa504]

As anyway this condition needs to be checked inside normalizeListKeyPath, so not performed check again here.

[elek]

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.

[bharatviswa504]

Thanks to explain it. I understood your point. Updated the code.

[elek]

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)

[bharatviswa504]

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.

[elek]

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.

[elek]

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?)

[bharatviswa504]

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.

[bharatviswa504]

Thank You @elek for the review.
I have addressed the review comments.

[rakeshadr]

+1 LGTM
Thanks @bharatviswa504 for the contribution.

[bharatviswa504]

Thank You @rakeshadr and @elek for the review.
@elek if you have any more comments, happy to address them in a new Jira.