-
Notifications
You must be signed in to change notification settings - Fork 621
HDDS-11484. Add check for javadoc correctness #7245
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
Changes from 37 commits
35f301f
32a30fd
9b7b0f2
f3cfffe
5bf24d4
c66f57e
62f70e4
5a3101d
ecdebda
7b27171
a358800
7db59c1
5221843
4ca55cb
f46675e
db67083
abb7b54
4a1b4c4
fbb6bb0
4d8a118
c650234
6f60dcb
3ae1f5e
8a53c5d
1959040
1fa2082
b0b5a38
db6296b
f595741
3ab7fea
7daaeb5
1e01abb
2bacc63
e959185
d353bed
e09a5d1
a43c9dc
86dac52
f377ac0
96e349b
18c7e2f
cb84092
499b2d8
6a35155
c2a3c37
5dbfca8
22697d3
12523eb
97160fd
be2eb27
d7ca1b2
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -152,7 +152,7 @@ protected int calculateExpectedDataBlocks(ECReplicationConfig rConfig) { | |
| * Using the current position, returns the index of the blockStream we should | ||
| * be reading from. This is the index in the internal array holding the | ||
| * stream reference. The block group index will be one greater than this. | ||
| * @return | ||
| * @return {@code int} | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This kind of description is redundant, return type is shown in method signature. If no meaningful description can be added, we can remove the
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||
| */ | ||
| protected int currentStreamIndex() { | ||
| return (int)((position / ecChunkSize) % repConfig.getData()); | ||
|
|
@@ -206,7 +206,7 @@ protected BlockExtendedInputStream getOrOpenStream(int locationIndex) throws IOE | |
| * to the replicaIndex given based on the EC pipeline fetched from SCM. | ||
| * @param replicaIndex | ||
| * @param refreshFunc | ||
| * @return | ||
| * @return {@code Function<BlockID, BlockLocationInfo>} | ||
| */ | ||
| protected Function<BlockID, BlockLocationInfo> ecPipelineRefreshFunction( | ||
| int replicaIndex, Function<BlockID, BlockLocationInfo> refreshFunc) { | ||
|
|
@@ -241,7 +241,7 @@ protected Function<BlockID, BlockLocationInfo> ecPipelineRefreshFunction( | |
| * potentially partial last stripe. Note that the internal block index is | ||
| * numbered starting from 1. | ||
| * @param index - Index number of the internal block, starting from 1 | ||
| * @return | ||
| * @return {@code long} | ||
| */ | ||
| protected long internalBlockLength(int index) { | ||
| long lastStripe = blockInfo.getLength() % stripeSize; | ||
|
|
@@ -344,7 +344,7 @@ protected boolean shouldRetryFailedRead(int failedIndex) { | |
| * strategy buffer. This call may read from several internal BlockInputStreams | ||
| * if there is sufficient space in the buffer. | ||
| * @param strategy | ||
| * @return | ||
| * @return {@code int} | ||
| * @throws IOException | ||
| */ | ||
| @Override | ||
|
|
@@ -409,7 +409,7 @@ protected void seekStreamIfNecessary(BlockExtendedInputStream stream, | |
| * group length. | ||
| * @param stream Stream to read from | ||
| * @param strategy The ReaderStrategy to read data into | ||
| * @return | ||
| * @return {@code int} | ||
| * @throws IOException | ||
| */ | ||
| private int readFromStream(BlockExtendedInputStream stream, | ||
|
|
||
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -20,11 +20,12 @@ | |||||
|
|
||||||
| /** | ||||||
| * This class contains constants for Recon related configuration keys used in | ||||||
| * SCM & Datanode. | ||||||
| * SCM & Datanode. | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. nit: "and" is simpler, and more readable in code view.
Suggested change
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||||||
| */ | ||||||
| public final class ReconConfigKeys { | ||||||
|
|
||||||
| /** | ||||||
| /**This class contains constants for Recon related configuration keys used in | ||||||
| * SCM & Datanode. | ||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why do we need to repeat the class description? Javadoc comment can be converted to regular code comment on private constructor.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done |
||||||
| * Never constructed. | ||||||
| */ | ||||||
| private ReconConfigKeys() { | ||||||
|
|
@@ -71,7 +72,7 @@ private ReconConfigKeys() { | |||||
| * Recon administrator users delimited by a comma. | ||||||
| * This is the list of users who can access admin only information from recon. | ||||||
| * Users defined in | ||||||
| * {@link org.apache.hadoop.ozone.OzoneConfigKeys.OZONE_ADMINISTRATORS} | ||||||
| * {@link org.apache.hadoop.ozone.OzoneConfigKeys#OZONE_ADMINISTRATORS} | ||||||
| * will always be able to access all recon information regardless of this | ||||||
| * setting. | ||||||
| */ | ||||||
|
|
||||||
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.
Javadoc generation is already enabled in
compilecheck, so this check performs duplicate work. We only need to setmaven.javadoc.failOnWarnings=trueincompileto catch javadoc problems.ozone/.github/workflows/ci.yml
Line 221 in a43c9dc
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.
The problem here is that when we compile on Java 8 some unexpected warnings appear, like https://github.com/Daniilchik/ozone/actions/runs/11087840906/job/30809312296#step:8:12947
It only appears on Java 8 and actually there's a lot more problems with Javadoc generation on this version, for example we can't
@linkto inner class, see: https://stackoverflow.com/a/50882307So that's why if we need to
failOnWarningsit’s better to generate Javadoc using a different version of Java rather than version 8.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.
compilecheck runs with Java 11, 17, 21, and with 8 on Mac. We can probably setfailOnWarningsconditionally (something like-Dmaven.javadoc.failOnWarnings=${{ matrix.java != 8 }}).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.
Thanks for the advice, done.