HADOOP-18156. Address JavaDoc warnings in classes like MarkerTool, S3ObjectAttributes, etc #4965
Conversation
This file contains 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
dannycjones
suggested changes
Oct 4, 2022
| public ScanArgsBuilder withLimit(final int l) { | ||
| this.limit = l; | ||
| return this; | ||
| } | ||
|
|
||
| /** Consider only markers in nonauth paths as errors. */ | ||
| /** Consider only markers in non-authoritative paths as errors. | ||
| * @param b True if tool should only consider markers in non-authoritative paths |
There was a problem hiding this comment.
nit: indentation
can you drop the extra space from this line, commit, and push?
dannycjones
approved these changes
Oct 4, 2022
There was a problem hiding this comment.
+1 (non-binding) assuming Yetus is happy, lgtm! Thanks for this @sauraank.
|
Hey @mukund-thakur, do you have time this week to review this? |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
dannycjones
approved these changes
Oct 4, 2022
There was a problem hiding this comment.
looks good. Just some minor comments post that will merge. Thanks @sauraank for picking this up.
| @@ -37,6 +37,7 @@ | |||
| import com.amazonaws.services.s3.model.MultiObjectDeleteException; | |||
| import org.apache.hadoop.classification.VisibleForTesting; | |||
| import org.apache.hadoop.util.Preconditions; | |||
|
|
|||
There was a problem hiding this comment.
Let's not add a new line here. Will cause backporting issue.
| @@ -960,43 +960,64 @@ public static final class ScanArgsBuilder { | |||
| /** Consider only markers in nonauth paths as errors. */ | |||
| private boolean nonAuth = false; | |||
|
|
|||
| /** Source FS; must be or wrap an S3A FS. */ | |||
| /** Source FS; must be or wrap an S3A FS. | |||
There was a problem hiding this comment.
the comment should start in newline not in the **. The same goes for all the comments below at multiple places.
There was a problem hiding this comment.
Do you think it's a good idea adding this to CheckStyle?
This one looks relevant (though maybe it will flag a lot of existing files): https://checkstyle.sourceforge.io/config_javadoc.html#JavadocContentLocation
There was a problem hiding this comment.
Started all the comments from new line MarkerTool.java. Thanks Mukund and Danny for the feedback.
There was a problem hiding this comment.
Let's not change the check style as yes it will flag many issues.
There was a problem hiding this comment.
Well, when there is a single line comment we don't need a new line but a new line is required when there are multiple lines comments as explained in the check style rule sent by Danny. This is based on my observation in the Apache Hadoop codebase.
Although I think the current changes should be okay, will confirm with @steveloughran once and commit.
Sorry for not being clear before.
There was a problem hiding this comment.
Let's not change the check style as yes it will flag many issues.
It will flag issues, but maybe worth it to avoid needing to add this sort of feedback in future.
Let Yetus tell us before we review.
If we were to consider it, I'd propose to do it:
- In a separate PR / task so we can acknowledge "yes this is adding new warnings".
- Only for
hadoop-awsmodule.
There was a problem hiding this comment.
Sure we can try only for hadoop-aws module.
|
💔 -1 overall
This message was automatically generated. |
steveloughran
requested changes
Oct 11, 2022
There was a problem hiding this comment.
-1 to splitting one line javadoc description of fields; that's a common style used throughout the codebase and other apps.
for methods with @return values, different story, and makes sense if the javadoc tool is happy.
do view this stuff in intellij set to format javadocs, if you haven't already. it will show why
elements do make a big difference in readability of formatted docs.
| @@ -34,20 +34,20 @@ | |||
| * Tracks directory markers which have been reported in object listings. | |||
| * This is needed for auditing and cleanup, including during rename | |||
| * operations. | |||
| * <p></p> | |||
| * | |||
There was a problem hiding this comment.
these should actually be
without the closing element, but retained.
if you set your IDE to format javadocs, it will then preserve the line break
There was a problem hiding this comment.
Thanks Steve! Will work on your recommended changes.
| /** Number of markers deleted. */ | ||
| /** | ||
| * Number of markers deleted. | ||
| */ |
There was a problem hiding this comment.
no need to do this for fields; leave as is
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
💔 -1 overall
This message was automatically generated. |
|
@steveloughran thanks for the review. I have made the changes of keeping the one line comment for fields. I have also added the tag for new line in JavaDoc. |
dannycjones
approved these changes
Oct 17, 2022
There was a problem hiding this comment.
Thanks for working in the feedback, @sauraank.
+1 (non-binding)
steveloughran
approved these changes
Oct 17, 2022
| @@ -34,20 +34,20 @@ | |||
| * Tracks directory markers which have been reported in object listings. | |||
| * This is needed for auditing and cleanup, including during rename | |||
| * operations. | |||
| * <p></p> | |||
| * <p> | |||
| @@ -840,6 +846,7 @@ public void setVerbose(final boolean verbose) { | |||
| * Execute the marker tool, with no checks on return codes. | |||
| * | |||
| * @param scanArgs set of args for the scanner. | |||
| * @throws IOException IO failure | |||
There was a problem hiding this comment.
can you put this below the @return line?
steveloughran
pushed a commit
to steveloughran/hadoop
that referenced
this pull request
Oct 17, 2022
…ObjectAttributes, etc (apache#4965) Contributed by Ankit Saurabh
4 tasks
steveloughran
added a commit
that referenced
this pull request
Oct 20, 2022
…ObjectAttributes, etc (#4965) Contributed by Ankit Saurabh
HarshitGupta11
pushed a commit
to HarshitGupta11/hadoop
that referenced
this pull request
Nov 28, 2022
…ObjectAttributes, etc (apache#4965) Contributed by Ankit Saurabh
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Description of PR
Fixed the Javadoc warnings that were there in MarkerTool.java, DirectoryPolicy.java, DirMarkerTracker.java and OperationCallbacks.java
How was this patch tested?
Checked the JavaDoc warnings during build