Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fail build on missing Javadocs #45

Merged
merged 17 commits into from
Jul 14, 2022
Merged

Fail build on missing Javadocs #45

merged 17 commits into from
Jul 14, 2022

Conversation

dbwiddis
Copy link
Member

@dbwiddis dbwiddis commented Jul 11, 2022 •
edited
Loading

Signed-off-by: Daniel Widdis [email protected]

Description

  • Fails the build on missing Javadocs
  • Adds the missing JavaDocs so build doesn't fail

Javadocs:

  • ActionListener
  • ExtensionsRunner
  • ClusterSettingsResponseHandler
  • LocalNodeResponseHandler
  • ClusterStateResponseHandler
  • ExtensionSettings

Excluded Netty4 code as it is here temporarily; it will be eventually published to the central repository via OpenSearch, see opensearch-project/OpenSearch#3118

Issues Resolved

Fixes #27

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

@dbwiddis
Fail build on missing Javadocs
e9c0a18 
Signed-off-by: Daniel Widdis <[email protected]>
dbwiddis
dbwiddis commented Jul 12, 2022
View reviewed changes
build.gradle Outdated Show resolved Hide resolved
build.gradle Show resolved Hide resolved
gradle/missing-javadoc.gradle Outdated Show resolved Hide resolved
peternied
peternied reviewed Jul 12, 2022
View reviewed changes
Copy link
Member

@peternied peternied left a comment

Choose a reason for hiding this comment

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

I scoped my comments to the documentation that was added

src/main/java/org/opensearch/sdk/ActionListener.java Show resolved Hide resolved
src/main/java/org/opensearch/sdk/ActionListener.java Outdated Show resolved Hide resolved
src/main/java/org/opensearch/sdk/ActionListener.java Show resolved Hide resolved
@dbwiddis
Javadocs for ExtensionsRunner
c5758e4 
Signed-off-by: Daniel Widdis <[email protected]>
peternied
peternied reviewed Jul 12, 2022
View reviewed changes
src/main/java/org/opensearch/sdk/ExtensionsRunner.java Outdated Show resolved Hide resolved
src/main/java/org/opensearch/sdk/ExtensionsRunner.java
@@ -85,6 +93,12 @@ private DiscoveryNode getOpensearchNode() {
return opensearchNode;
}

/**
* Handles a plugin request from OpenSearch. This is the first request for the transport communication and will initialize the extension and will be a part of OpenSearch bootstrap.
Copy link
Member

Choose a reason for hiding this comment

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

This is the first request

It would by very valuable to have the full lifecycle of this process documented, maybe here? Then additional parts of the process would link back to this comment with a @see annotation

Copy link
Member

Choose a reason for hiding this comment

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

Just to give some context here. During bootstrap OpenSearch will look for any extensions available running on a different port. Once discovered, read their extensions.yml file and get the metadata of that extension.

src/main/java/org/opensearch/sdk/ExtensionsRunner.java Show resolved Hide resolved
src/main/java/org/opensearch/sdk/ExtensionsRunner.java Show resolved Hide resolved
@peternied
Copy link
Member

Thanks for iterating on this @dbwiddis, there is a lot of surface area here and its great to get a fine tooth comb looking over it and improvement maintainability of the codebase

dbwiddis added 8 commits July 12, 2022 17:05
@dbwiddis
Replace missing-javadoc and missing-doclet with require-javadoc
f428226 
Signed-off-by: Daniel Widdis <[email protected]>
@dbwiddis
Doc updates based on code review.
31a5e9d 
Signed-off-by: Daniel Widdis <[email protected]>
@dbwiddis
Add javadocs for ResponseHandler classes
9bfb009 
Signed-off-by: Daniel Widdis <[email protected]>
@dbwiddis
Add javadocs to ExtensionSettings
9254062 
Signed-off-by: Daniel Widdis <[email protected]>
@dbwiddis
Disable javadocStrict check on netty4 package
b9a3ef3 
Signed-off-by: Daniel Widdis <[email protected]>
@dbwiddis
Reenable strict doc checks on netty4, fix errors
73d18a2 
Signed-off-by: Daniel Widdis <[email protected]>
@dbwiddis
Final tweaks
20ce088 
Signed-off-by: Daniel Widdis <[email protected]>
@dbwiddis
Fix missing Netty4 params causing failure
d87e4ae 
Signed-off-by: Daniel Widdis <[email protected]>
@dbwiddis dbwiddis marked this pull request as ready for review July 13, 2022 15:20
@dbwiddis dbwiddis requested a review from a team July 13, 2022 15:20
@dbwiddis
Copy link
Member Author

dbwiddis commented Jul 13, 2022 •
edited
Loading

Ready for review:

  • Need two separate checks. One to find missing javadocs, the other to strictly check whether those javadocs are correct.
    • Finding missing javadocs implemented with org.plumelib:require-javadoc:1.0.3. I'll be able to tweak the gradle build with 1.0.4, so probably okay to wait to merge this.
    • Strict checks using the javadoc tool with -XWerror and -Xdoclint:all. Produces some warnings associated with things we're intentionally skipping (undocumented default constructors, getters & settors), but only errors fail the build.
  • For now I'm skipping all the missing javadocs in the netty package (I did fix the malformed ones.). They can be added later if any of that code is going to be permanent, but the state of the code implied at least some of it is temporary.

saratvemulapalli
saratvemulapalli previously approved these changes Jul 13, 2022
View reviewed changes
Copy link
Member

@saratvemulapalli saratvemulapalli left a comment

Choose a reason for hiding this comment

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

This is a great start.
Thanks @dbwiddis !!

@dbwiddis
Copy link
Member Author

Added back the exclusion for netty4 and added the tracking issue. So now this should be mergeable.

owaiskazi19
owaiskazi19 reviewed Jul 13, 2022
View reviewed changes
build.gradle Outdated Show resolved Hide resolved
owaiskazi19
owaiskazi19 reviewed Jul 13, 2022
View reviewed changes
build.gradle Outdated Show resolved Hide resolved
owaiskazi19
owaiskazi19 reviewed Jul 13, 2022
View reviewed changes
src/main/java/org/opensearch/sdk/ExtensionsRunner.java
* @param settings The transport settings to configure.
* @param threadPool A thread pool to use.
* @return The configured Netty4Transport object.
*/
public Netty4Transport getNetty4Transport(Settings settings, ThreadPool threadPool) {
Copy link
Member

Choose a reason for hiding this comment

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

For later: Can change this method to createNetty4Transport.

@dbwiddis dbwiddis mentioned this pull request Jul 13, 2022
Closed
@dbwiddis @owaiskazi19
Update comments per code review
c97bb82 
Co-authored-by: Owais Kazi <[email protected]>
Signed-off-by: Daniel Widdis <[email protected]>
owaiskazi19
owaiskazi19 reviewed Jul 14, 2022
View reviewed changes
Copy link
Member

@owaiskazi19 owaiskazi19 left a comment

Choose a reason for hiding this comment

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

Not sure if part of this PR but we need OpenSearch license header for all the files

@dbwiddis dbwiddis requested a review from owaiskazi19 July 14, 2022 05:24
@dbwiddis
Copy link
Member Author

Not sure if part of this PR but we need OpenSearch license header for all the files

Make a new issue for this. There are license-check plugins.

owaiskazi19
owaiskazi19 approved these changes Jul 14, 2022
View reviewed changes
Copy link
Member

@owaiskazi19 owaiskazi19 left a comment

Choose a reason for hiding this comment

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

Overall LGTM! Thanks @dbwiddis for all the changes

@owaiskazi19
Copy link
Member

Not sure if part of this PR but we need OpenSearch license header for all the files

Make a new issue for this. There are license-check plugins.

#50

@dbwiddis dbwiddis merged commit 4a3ac6d into opensearch-project:main Jul 14, 2022
@dbwiddis dbwiddis deleted the javadocs branch July 14, 2022 18:47
@owaiskazi19 owaiskazi19 mentioned this pull request Sep 7, 2022
Merged
kokibas pushed a commit to kokibas/opensearch-sdk-java that referenced this pull request Mar 17, 2023
@dbwiddis @owaiskazi19 @kokibas
Fail build on missing Javadocs (opensearch-project#45)
7c1188b 
* Fail build on missing Javadocs

Signed-off-by: Daniel Widdis <[email protected]>

* Fix build errors from package rename

Signed-off-by: Daniel Widdis <[email protected]>

* Javadocs for ActionListener

Signed-off-by: Daniel Widdis <[email protected]>

* Javadocs for ExtensionsRunner

Signed-off-by: Daniel Widdis <[email protected]>

* Replace missing-javadoc and missing-doclet with require-javadoc

Signed-off-by: Daniel Widdis <[email protected]>

* Doc updates based on code review.

Signed-off-by: Daniel Widdis <[email protected]>

* Add javadocs for ResponseHandler classes

Signed-off-by: Daniel Widdis <[email protected]>

* Add javadocs to ExtensionSettings

Signed-off-by: Daniel Widdis <[email protected]>

* Disable javadocStrict check on netty4 package

Signed-off-by: Daniel Widdis <[email protected]>

* Reenable strict doc checks on netty4, fix errors

Signed-off-by: Daniel Widdis <[email protected]>

* Final tweaks

Signed-off-by: Daniel Widdis <[email protected]>

* Fix missing Netty4 params causing failure

Signed-off-by: Daniel Widdis <[email protected]>

* Don't use Werror, include strict check on private javadocs if present

Signed-off-by: Daniel Widdis <[email protected]>

* Exclude netty4 from checks and link to tracking issue

Signed-off-by: Daniel Widdis <[email protected]>

* Clarify and add links on future action for temporary exclusions

Signed-off-by: Daniel Widdis <[email protected]>

* Update comments per code review

Co-authored-by: Owais Kazi <[email protected]>
Signed-off-by: Daniel Widdis <[email protected]>

* Update require-javadoc version, fix get/set typo

Signed-off-by: Daniel Widdis <[email protected]>

Co-authored-by: Owais Kazi <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@peternied peternied peternied left review comments

@saratvemulapalli saratvemulapalli saratvemulapalli approved these changes

@owaiskazi19 owaiskazi19 owaiskazi19 approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[FEATURE] Add Java docs and mandate for all new classes
4 participants
@dbwiddis @peternied @owaiskazi19 @saratvemulapalli