-
Notifications
You must be signed in to change notification settings - Fork 8
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
Change Alluxio configuration approach 2 #50
Conversation
@SibylYang could you help resolve the conflicts with main |
test_options=None, | ||
**kwargs, | ||
): | ||
""" | ||
Initializes an Alluxio filesystem on top of underlying filesystem | ||
to leveraging the data caching and management features of Alluxio. | ||
|
||
The Alluxio args: | ||
etcd_hosts (str, optional): A comma-separated list of ETCD server hosts in the format "host1:port1,host2:port2,...". |
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.
Should we keep the alluxiofs doc? this is because s3fs, although the parameters are passed in **kwargs but still in the doc, user may view this doc directly for supported parameters? but it will be hard to maintain as well
@jja725 what are your thoughts?
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.
yeah agree let's keep the doc here otherwise no one can find the proper options to use
@@ -10,7 +10,7 @@ def fs(self): | |||
return AlluxioFileSystem( | |||
etcd_hosts="localhost", | |||
target_protocol="s3", | |||
test_options={"skip_alluxio": True}, |
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.
@SibylYang the reason we adding test_options is to put some shaded values here. please do not modify the user behavior for now because some of our users are relying on the existing configurations.
Please do not modify the end-user behavior
Built upon previous PR: #23. Addressed comments regarding changing to asserts in config.py.