HADOOP-18987 Various fixes to FileSystem API docs #6292
Conversation
|
🎊 +1 overall
This message was automatically generated. |
There was a problem hiding this comment.
DIeter, your rigorousness is appreciated! The spec does try to balance some formal specification with python syntax so developers read it. We know that things like TLA+ don't work as nobody ever files bug reports against those files.
What is key is that there is that strong model underneath and that we can use the precond/postcond clauses to write our compliance tests. And we have somewhere to document how nobody understands what rename() does.
Made some minor suggestions
| @@ -87,15 +87,14 @@ for example. output streams returned by the S3A FileSystem. | |||
|
|
|||
| The stream MUST implement `Abortable` and `StreamCapabilities`. | |||
|
|
|||
| ```python | |||
| if unsupported: | |||
| ``` | |||
There was a problem hiding this comment.
can you restore this. yes, it bears more relation to the Z specification language of Ince93 etc, but if you tag as Python then IDEs cover it. And Github doesn't really handle any of the formal languages. Nor do most developers, which is why this uses a python-esque syntax.
There was a problem hiding this comment.
Ah, I considered the python tags detrimental because my IDE (Intellij) had totally incorrect syntax highlights for these snippets, because they are not valid Python. Specifically, the FS' causes the IDE to think a string is being defined.
But I'm fine with re-applying the python tags.
There was a problem hiding this comment.
I know it's not well formed python, but neither are our java or xml snippets in the docs. consider it best-effort
| @@ -432,7 +432,7 @@ of data which must be collected in a single RPC call. | |||
|
|
|||
| #### Preconditions | |||
|
|
|||
| exists(FS, path) else raise FileNotFoundException | |||
| if not exists(FS, path) : raise FileNotFoundException | |||
There was a problem hiding this comment.
I was using the concept exists() holds or raise FNFE, but if this suits then I'm not going to argue about details. The goal of this spec is for other people and tests, not mathematical purism.
There was a problem hiding this comment.
Either form is understandable imo, just thought the suggested form is more in line with the remainder of the docs which uses pythonesque code.
| @@ -88,7 +88,7 @@ Get the status of a path | |||
| stat.length = len(FS.Files[p]) | |||
| stat.isdir = False | |||
| stat.blockSize > 0 | |||
| elif isDir(FS, p) : | |||
| elif isDirectory(FS, p) : | |||
There was a problem hiding this comment.
isDir(FS, Path) is the function in the model; 'isDirectory()` path the published method in Filesystem.
There was a problem hiding this comment.
Indeed, missed that. Also changed line 67 to reflect that.
|
|
||
| The result provides access to the byte array defined by `FS.Files[p]`; whether that | ||
| The result provides access to the byte array defined by `FS'.Files[p]`; whether that |
There was a problem hiding this comment.
open() is not mutating; FS` = FS; therefore FS is all that is needed
There was a problem hiding this comment.
Aha, I then misinterpreted
The suffix “’” (single quote) is used as the convention to indicate the state of the system after an operation
on https://hadoop.apache.org/docs/stable/hadoop-project-dist/hadoop-common/filesystem/notation.html
I'll add a small clarification there, and revert this.
hadoop-common-project/hadoop-common/src/site/markdown/filesystem/filesystem.md
Outdated
Show resolved
Hide resolved
hadoop-common-project/hadoop-common/src/site/markdown/filesystem/model.md
Outdated
Show resolved
Hide resolved
hadoop-common-project/hadoop-common/src/site/markdown/filesystem/model.md
Outdated
Show resolved
Hide resolved
hadoop-common-project/hadoop-common/src/site/markdown/filesystem/filesystem.md
Outdated
Show resolved
Hide resolved
DieterDP-ng
force-pushed
the
HADOOP-18987
branch
from
423596f to
a044705
Compare
DieterDP-ng
force-pushed
the
HADOOP-18987
branch
from
a044705 to
ff10a23
Compare
|
dieter, always best to not do a rebase and force push unless there's incompatible changes...makes it harder to see the diff. I know, I do it especially on long-lived PRs when other people (stevel for example) makes changes in the same place -but I know it causes pain for all. merge commits aren't so bad |
|
💔 -1 overall
This message was automatically generated. |
|
@DieterDP-ng before I merge, what name do you want to be used in the commit message for credit? |
|
... never mind, found your full name on the JIRA. |
|
🎊 +1 overall
This message was automatically generated. |
Contributed by Dieter De Paepe
Contributed by Dieter De Paepe
Description of PR
This PR contains documentation updates to the Hadoop FileSystem API definition. No intention of the docs is changed, just inconsistencies, informal statements, typos etc.
How was this patch tested?
Not tested - only contains docs.