Skip to content

HDDS-9804. ozone freon dfsg does not work with relative --path#5704

Merged
adoroszlai merged 2 commits intoapache:masterfrom
jojochuang:HDDS-9804
Dec 7, 2023
Merged

HDDS-9804. ozone freon dfsg does not work with relative --path#5704
adoroszlai merged 2 commits intoapache:masterfrom
jojochuang:HDDS-9804

Conversation

@jojochuang
Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

Add check to prevent invalid --path parameter in ozone freon dfsg command.

Please describe your PR in detail:

  • It's a trivial change but took me several hours to troubleshoot so thought it's worth fixing.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-9804

How was this patch tested?

Manually tested on a cluster.

Change-Id: I5829de1d89dbc203e41d7c07d310435e1d683eb5
Copy link
Copy Markdown
Contributor

@nandakumar131 nandakumar131 left a comment

Choose a reason for hiding this comment

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

+1, LGTM.

Comment on lines +90 to +93
String scheme = uri.getScheme();
if (scheme == null) {
throw new IllegalArgumentException("--path requires FQDN");
}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Shouldn't we use the default FS in that case? If no FS is specified explicitly, the defaultFs should be used, right? Something like this:

    if (scheme == null) {
      URI defaultUri = FileSystem.getDefaultUri(configuration)
      uri = new URI(defaultUri.getScheme(), defaultUri.getAuthority(), uri.getPath());
    }

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We don't even need to completely replace uri, can use scheme of default URI only for disabling the cache.

Comment on lines +90 to +91
String scheme = uri.getScheme();
if (scheme == null) {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

does this work?

    if (!uri.isAbsolute()) {

Copy link
Copy Markdown
Contributor

@adoroszlai adoroszlai left a comment

Choose a reason for hiding this comment

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

Thanks @jojochuang for finding this bug, and working on the fix.

file system cache is not disabled properly and all threads share the same FS object

Instead of forcing full URLs, we should fix the way cache is disabled.

Comment on lines +90 to +93
String scheme = uri.getScheme();
if (scheme == null) {
throw new IllegalArgumentException("--path requires FQDN");
}
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We don't even need to completely replace uri, can use scheme of default URI only for disabling the cache.

Comment on lines 94 to 95
String disableCacheName = String.format("fs.%s.impl.disable.cache",
uri.getScheme());
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Suggested change
String disableCacheName = String.format("fs.%s.impl.disable.cache",
uri.getScheme());
String scheme = Optional.ofNullable(uri.getScheme())
.orElseGet(() -> FileSystem.getDefaultUri(configuration).getScheme());
String disableCacheName = String.format("fs.%s.impl.disable.cache", scheme);

(also add import java.util.Optional)

Or can use if block, if you prefer.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Maybe should also fail if scheme is file.

Copy link
Copy Markdown
Contributor

@adoroszlai adoroszlai left a comment

Choose a reason for hiding this comment

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

Thanks @jojochuang for updating the patch.

@adoroszlai adoroszlai changed the title HDDS-9804. ozone freon dfsg --path must be FQDN. HDDS-9804. ozone freon dfsg does not work with relative --path Dec 7, 2023
@adoroszlai adoroszlai requested a review from ayushtkn December 7, 2023 04:07
Copy link
Copy Markdown
Member

@ayushtkn ayushtkn left a comment

Choose a reason for hiding this comment

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

LGTM

@adoroszlai adoroszlai merged commit ee8dd8c into apache:master Dec 7, 2023
@adoroszlai
Copy link
Copy Markdown
Contributor

Thanks @jojochuang for the patch, @ayushtkn, @nandakumar131 for the review.

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