Skip to content

Conversation

@duongkame
Copy link
Contributor

What changes were proposed in this pull request?

Problem

The target of this task is to deprecate the LookupFile interface and use GetKeyInfo for the OFS use case.

The gap between OFS and object storage use cases (like S3) is that due to a file system nature, OFS has to answer the file read requests from its client. A file read input is a file path and OFS needs to be able to decide if the given path is a valid file. To be specific, if the given path is not a file (but a directory), OFS should return an error. Reading empty files is a valid use case.

// throws IOException if the given path is not a file.
FileSystem.open(path)

The lookupKey API returns the same content for an empty file and directory.

Today, this is solved in Ozone by the separated lookupFile API, detecting if the given key is a directory and throwing an error on server-side.

throw new OMException(ResultCodes.NOT_A_FILE);

GetKeyInfo replaces lookupKey and inherits the same limit.

Solution

To make GetKeyInfo available for OFS, we can simply add to its response a field indicating if the key represents a directory or a file so that OFS client code can make the assertion.

https://issues.apaceorg/jira/browse/HDDS-7419

How was this patch tested?

OFS integration test.

@duongkame duongkame marked this pull request as ready for review October 28, 2022 18:45
@duongkame duongkame changed the title Hdds 7419 HDDS-7419. Integrate the GetKeyInfo API to OFS Oct 28, 2022
if (omVersion.compareTo(OzoneManagerVersion.OPTIMIZED_GET_KEY_INFO) >= 0) {
keyInfo = ozoneManagerClient.getKeyInfo(keyArgs, false)
.getKeyInfo();
if (!keyInfo.isFile()) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to reuse getKeyInfo and check isFile if there was no exception (client used lookup and it failed)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's possible.

However, if we just reused getKeyInfo, then in turn it would call lookupKey for older OM and that would not result in either isFile attribute or NOT_A_FILE exception. (For older OM, clients have to call lookupFile).

Yet, to make things right, we can make getKeyInfo looks like the following to make it reusable across object storage and OFS use cases.

private OmKeyInfo getKeyInfo(String volumeName, String bucketName, String keyName,
      boolean forceUpdateContainerCache, boolean lookupFile) {
  if (newOM) { call getKeyInfo }
  else if (lookupFile) { call lookupFile }
  else { call lookupKey }
}

I feel that would just move complexity from one place to another.

Copy link
Contributor

@DaveTeng0 DaveTeng0 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(except Ritesh's suggestion) otherwise LGTM!

Copy link
Contributor

@jojochuang jojochuang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

qq: Will it work as expected if a new client connects to an old OM?
I suspect this scenario wouldn't work but want to double check with you.

Thanks


String dirKey = OzoneFSUtils.addTrailingSlashIfNeeded(keyName);
OmKeyInfo dirKeyInfo = getOmKeyInfo(volumeName, bucketName, dirKey);
if (dirKeyInfo != null) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

how about adding an assertion here that dirKeyInfo.isFile() is false?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That also works, just like the lookupFile does today. Yet, I think getKeyInfo should be a generic API that provide key information, like its name.

The decision of how to use that information should be in the hands of OM's clients. E.g. RpcClient should know it's looking for a file instead of relying on OM to decide.

@duongkame
Copy link
Contributor Author

qq: Will it work as expected if a new client connects to an old OM? I suspect this scenario wouldn't work but want to double check with you.

Thanks

Thanks for the review, @jojochuang.
So, when new clients connect to old OM, they will call the existing lookupFile API. There's a test for that in the xcompat acceptance I believe.

Copy link
Contributor

@jojochuang jojochuang left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1 from me. Merging now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants