-
Notifications
You must be signed in to change notification settings - Fork 2.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[ALLUXIO-2546] Improve fault tolerance + HDFS API quality of life #4714
Conversation
|
||
super.initialize(uri, conf); | ||
LOG.debug("initialize({}, {}). Connecting to Alluxio", uri, conf); | ||
HadoopUtils.addS3Credentials(conf); | ||
HadoopUtils.addSwiftCredentials(conf); | ||
setConf(conf); | ||
mAlluxioHeader = getScheme() + "://" + uri.getHost() + ":" + uri.getPort(); | ||
|
||
String authority = uri.getHost() + ":" + uri.getPort(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do you want to add comments that explain what is going on here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does getAuthority
work better?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
have you addressed @calvinjia's comment as well?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done done
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changes look good, but it would be good to document the difference between the two modes of operation
Merged build finished. Test PASSed. |
Test PASSed. |
Automated checks report:
All checks passed! |
@@ -422,15 +422,24 @@ public Path getWorkingDirectory() { | |||
@SuppressFBWarnings("ST_WRITE_TO_STATIC_FROM_INSTANCE_METHOD") | |||
@Override | |||
public void initialize(URI uri, org.apache.hadoop.conf.Configuration conf) throws IOException { | |||
Preconditions.checkNotNull(uri.getHost(), PreconditionMessage.URI_HOST_NULL); | |||
Preconditions.checkNotNull(uri.getPort(), PreconditionMessage.URI_PORT_NULL); | |||
if (!Configuration.getBoolean(PropertyKey.ZOOKEEPER_ENABLED)) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add a comment that explains why is it okay to have empty host / port when using Zookeeper? thanks
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nevermind, just saw the documentation you added to file system API
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
comment added anyway
// requires that we use "alluxio-ft:///" for the header. | ||
authority = "/"; | ||
} | ||
// HDFS doesn't allow the authority to be ""; it must be "/" instead. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
replace ""
with empty
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
done
@calvinjia comments addressed, PTAL |
Merged build finished. Test PASSed. |
Test PASSed. |
Merged build finished. Test PASSed. |
Test PASSed. |
Merged build finished. Test PASSed. |
Test PASSed. |
LGTM |
https://alluxio.atlassian.net/browse/ALLUXIO-2546