Skip to content

Conversation

@zentol
Copy link
Contributor

@zentol zentol commented Feb 12, 2018

What is the purpose of the change

This PR ports the fileystem ConfigConstants to ConfigOptions and integrates them into the configuration docs generator.

Brief change log

  • port filesystem config constants to config options
  • Add missing descriptions to config options (derived from existing description/javadocs)
  • integrate filesystem configuration table into config.md

.key("fs.default-scheme")
.noDefaultValue();
.noDefaultValue()
.withDescription("The default filesystem scheme, used for paths that do not declare a scheme explicitly.");
Copy link
Contributor

Choose a reason for hiding this comment

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

Why does this not have the whole long text that the option has in the current documentation?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

IMO the missing text isn't really needed, but I'll add it back to keep this PR as a straight port of the existing docs and merge it afterwards.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ah I remember now, looking at the description:

[Part1]
The default filesystem scheme to be used

[Part2]
, with the necessary authority to contact, e.g. the host:port of the NameNode in the case of HDFS (if needed).

[Part3]
By default, this is set to file:/// which points to the local filesystem. This means that the local filesystem is going to be used to search for user-specified files without an explicit scheme definition.

[Part4]
This scheme is used ONLY if no other scheme is specified (explicitly) in the user-provided URI.

Part 1 and 4 are contained in the description, part 3 was left out since file:/// isn't the documented default. Only part 2 is really missing.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, because we have no default in here but isn't the FileSystem code using that as the default fallback?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, in a practical sense "file:///" is the default due to how FileSystem works. We can't define it as the official default at the moment because of OS compatibility, see LocalFileSystem:

LOCAL_URI = OperatingSystem.isWindows() ? URI.create("file:/") : URI.create("file:///");

Not sure whether that is really necessary though, but the scheme parsing by Paths is a bit wonky on Windows.

Copy link
Contributor

Choose a reason for hiding this comment

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

Ok, then I'd say merge it like this.

Copy link
Contributor

@aljoscha aljoscha left a comment

Choose a reason for hiding this comment

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

Looks good except that one question I had.

@zentol
Copy link
Contributor Author

zentol commented Feb 14, 2018

merging

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants