Conversation
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
Contributor
Author
|
My JIRA id is deepujain |
ctubbsii
suggested changes
Mar 9, 2026
ctubbsii
left a comment
Member
There was a problem hiding this comment.
+1 to the changes to the docs, and the addition of the test. However, it does not fully resolve the issue described in the JIRA issue. To fully resolve the issue, the FileSystem methods need to be fixed, because their trailing slashes do matter.
Contributor
Author
|
@ctubbsii thanks for the review. I have made additional changes. Please take another look when you have a moment. |
ctubbsii
reviewed
Mar 9, 2026
ctubbsii
left a comment
Member
There was a problem hiding this comment.
I think this is an improvement, but I had a question about how API changes should be marked in my below comments.
|
💔 -1 overall
This message was automatically generated. |
ctubbsii
approved these changes
Mar 10, 2026
Member
|
I really hate yetus being so spammy. It makes it hard to follow the human conversation and makes trying to contribute to Hadoop, even as a reviewer or reporter, extremely painful. Its comments should be collapsed by default. |
Contributor
Author
|
@ctubbsii Thanks for approving the MR. |
deepujain
force-pushed
the
HADOOP-19815-path-uri-trailing-slash-docs
branch
from
March 27, 2026 05:17
f817f26 to
04e6c16
Compare
Contributor
Author
|
Rebased on trunk. Fixed bootstrap null-working-directory handling in RawLocalFileSystem and ViewFileSystem, and updated the local filesystem home-directory expectation for directory-form URIs. Ran the impacted targeted Maven tests locally and they passed. Ready for CI. |
deepujain
force-pushed
the
HADOOP-19815-path-uri-trailing-slash-docs
branch
from
March 27, 2026 05:31
04e6c16 to
2beb43c
Compare
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
deepujain
force-pushed
the
HADOOP-19815-path-uri-trailing-slash-docs
branch
from
March 27, 2026 16:02
2beb43c to
3d9e7dc
Compare
Contributor
Author
|
Rebased onto trunk and backed out the broader FileSystem URI changes that were causing the common/viewfs regressions. Kept the Path(String) vs Path(URI) docs/tests, and verified |
|
🎊 +1 overall
This message was automatically generated. |
Contributor
Author
Member
|
@deepujain I would love for it to be merged and closed, but I am not a committer and don't have write access. |
Contributor
Author
|
CI is still green on the current head, and this branch has the narrowed scope plus prior approval. @steveloughran, when you have a moment, could you please take a look or point me to the right committer for this one? Happy to make any follow-up changes. |
steveloughran
requested changes
Jul 9, 2026
steveloughran
left a comment
Contributor
There was a problem hiding this comment.
move to assertj asserts to work with junit5 and backports
Member
|
@steveloughran wrote:
What does this mean? It doesn't seem related to this PR. |
Contributor
Author
|
Applied the latest review follow-up by moving the updated |
Contributor
Author
|
Applied the latest review follow-up on July 10, 2026 by moving the updated |
deepujain
force-pushed
the
HADOOP-19815-path-uri-trailing-slash-docs
branch
from
July 21, 2026 14:05
d4741ce to
aa92e1e
Compare
Contributor
Author
|
Rebased this branch onto Fresh local validation on the rebased branch: Leaving this here so the refreshed CI is tied to the current trunk head. @steveloughran, when you have a moment, could you please take another look? Happy to make any follow-up changes. |
|
🎊 +1 overall
This message was automatically generated. |
Contributor
Author
|
No code changes here. The latest review follow-up is already in, the branch was rebased onto current |
Member
|
Is @steveloughran the only active maintainer for this project that can merge a PR? This PR has been ready for months and @deepujain seems to be putting in a lot of work to keep it maintained in good faith. I don't think that's fair to them. Please merge this, or provide code reviews to suggest changes. |
Contributor
Author
|
The AssertJ follow-up from the July 9, 2026 review is already in, the branch was rebased onto current apache/trunk on July 21, 2026, and CI is green on the current head. @steveloughran, if anything is still pending from my side, I’m happy to address it. Otherwise, could you please take another look when you have a moment? |
Contributor
|
No, there are others, which is good as I'm inactive |
Member
Okay, sorry for tagging you then. You're the only one who has offered a code review. |
Contributor
|
Ask on the Hadoop developer mailing lists, specifically common-dev. The trouble with tagging people in PR is not only is it completely unexpected, is that it gets completely lost in the noise of all the fully automated messages we get from anything we submit ourselves. I think I only noticed this PR precisely because I'm not actually submitting any work anymore. |
deepujain
force-pushed
the
HADOOP-19815-path-uri-trailing-slash-docs
branch
from
September 1, 2026 16:05
aa92e1e to
5ab8595
Compare
Contributor
Author
|
Thanks for the guidance on outreach. I posted a review request to common-dev instead of tagging individuals here. I will not use @-mention reminders on this PR going forward. For anyone following the thread: the AssertJ follow-up from the July review is on the current head (5ab8595), the branch is rebased on current apache/trunk, and Yetus is green on the latest run. Scope is Path docs/tests only (broader FileSystem URI experiments were backed out after regressions). Discussion welcome here or on the list. |
|
🎊 +1 overall
This message was automatically generated. |
Contributor
Author
|
Hi @Hexiaoqiao @slfan1989 @pan3793, HADOOP-19815 (Path URI vs String docs) has been open since March. AssertJ follow-ups from the July review are on the current head and Yetus is +1. I also posted to common-dev@. A review or a quick 'wrong approach' would unblock me. |
deepujain
force-pushed
the
HADOOP-19815-path-uri-trailing-slash-docs
branch
from
September 3, 2026 17:58
5ab8595 to
5e8c414
Compare
|
🎊 +1 overall
This message was automatically generated. |
deepujain
force-pushed
the
HADOOP-19815-path-uri-trailing-slash-docs
branch
from
September 4, 2026 15:58
5e8c414 to
73543f4
Compare
Contributor
Author
|
Rebased cleanly onto current trunk at |
|
🎊 +1 overall
This message was automatically generated. |
Member
|
Have you reached out on the Hadoop mailing lists, as mentioned above? I think Hadoop committers aren't watching the GitHub activity from outside contributors. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Documents how
Pathhandles trailing slashes and how that affectsURI.resolve(). The string constructor removes trailing slashes from non-root paths, while the URI constructor preserves a trailing slash after normalization.The current scope is documentation and regression coverage only. Earlier
FileSystembehavior and public API changes were removed after review and regression findings.Change
Path.java: document trailing slash normalization for string and URI constructors, and recommend path concatenation when callers should not rely on URI resolution semantics.TestPath.java: cover constructor andURI.resolve()behavior.TestLocalFileSystem.java: preserve existing local filesystem path expectations.Evidence it works
JAVA_HOME=/opt/homebrew/opt/openjdk@17/libexec/openjdk.jdk/Contents/Home PATH=/opt/homebrew/opt/openjdk@17/libexec/openjdk.jdk/Contents/Home/bin:$PATH ./mvnw -Dmaven.repo.local=/private/tmp/hadoop-m2 test -pl hadoop-common-project/hadoop-common -am -Dtest=TestChRootedFileSystem,TestChRootedFs,TestViewFileSystemWithAuthorityLocalFileSystem,TestViewFsWithAuthorityLocalFs -DskipTests=falseJIRA
Fixes HADOOP-19815