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

DRILL-8162: Add OpenAPI Specification documentation for Drill's REST API #2489

Merged
merged 6 commits into from Mar 11, 2022
Merged

DRILL-8162: Add OpenAPI Specification documentation for Drill's REST API #2489

merged 6 commits into from Mar 11, 2022

Conversation

estherbuchwalter
Copy link
Contributor

DRILL-8162: Add OpenAPI Specification documentation for Drill's REST API

Description

Used Swagger Core to produce an OpenAPI specification for Drill's REST API. One can access it in JSON format by going to localhost:8047/openapi.json after starting Drill.
Also, added an HTML script to produce the Swagger UI when one visits localhost:8047/static/swagger-ui.html.
Used Swagger annotations to populate the Swagger UI with the relevant information, including links to documentation on https://drill.apache.org/docs/. More information can be added via additional annotations.

Documentation

One can visit localhost:8047/openapi.json after starting Drill to find an OpenAPI specification for Drill's REST API. One can also visit localhost:8047/static/swagger-ui.html after starting Drill to find the Swagger UI, which clearly displays the endpoints for the REST API with their descriptions and/or links to external docs.

Testing

Started Drill and visited the links above.

@cgivre
Copy link
Contributor

cgivre commented Mar 8, 2022 •
edited

@estherbuchwalter Can you please rebase on current master? The Travis CI failed for an unrelated reason, and I think @vdiravka had fixed this issue.
Thanks!

cgivre
cgivre requested changes Mar 9, 2022
View reviewed changes
Copy link
Contributor

@cgivre cgivre left a comment

Choose a reason for hiding this comment

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

@estherbuchwalter
Thanks for doing this. It looked like there are some changes from another PR that got incorporated into this. Please rebase on current master. Also, as a general comment, please revert the spacing and line breaks for long lines. It really helps for readability.

exec/java-exec/pom.xml Outdated Show resolved Hide resolved
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/DrillRestServer.java Outdated Show resolved Hide resolved
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/DrillRestServer.java Outdated Show resolved Hide resolved
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/DrillRoot.java Outdated
@@ -103,6 +109,8 @@ public Response getDrillbitStatus() {
@GET
@Path("/gracePeriod")
@Produces(MediaType.APPLICATION_JSON)
@Operation(externalDocs = @ExternalDocumentation(description = "Apache Drill REST API documentation:", url = " https://drill.apache.org/docs/stopping-drill/#:~:text=draining%2C%20and%20offline.%0A%20%20%20%20%20--,grace,-%3A%20A%20period%20in"))
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove extra spaces after the url tag. Here and elsewhere. Also, is it necessary to URL encode the links, or will Drill figure that out by itself?

If it is not necessary, I'd recommend rendering them normally, w/o the URL encoding.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The URL encoding came from copying the link to specific text on the page. Would you rather me remove the encoding and have the URLs point to the general page instead?

Copy link
Contributor

Choose a reason for hiding this comment

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

I think a link the general page is fine. If there is an internal reference IE:
https://somepage.com#part2
Definitely include that. But I'd remove the URL encoding if it isn't necessary.

exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/DrillRoot.java Show resolved Hide resolved
@cgivre cgivre added documentation PRs relating to documentation enhancement PRs that add a new functionality to Drill labels Mar 9, 2022
cgivre
cgivre requested changes Mar 10, 2022
View reviewed changes
Copy link
Contributor

@cgivre cgivre left a comment

Choose a reason for hiding this comment

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

@estherbuchwalter
Thanks for this. Could you please revert all spacing changes? It's hard to see what changes are actually being made when there are tons of spacing changes throughout core pieces of Drill.

There is a general comment about URL encoded links as well. Please avoid these if they are not necessary. Also you can remove the javascript bits from the links.

exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/DrillRoot.java Outdated
@@ -103,6 +109,8 @@ public Response getDrillbitStatus() {
@GET
@Path("/gracePeriod")
@Produces(MediaType.APPLICATION_JSON)
@Operation(externalDocs = @ExternalDocumentation(description = "Apache Drill REST API documentation:", url = " https://drill.apache.org/docs/stopping-drill/#:~:text=draining%2C%20and%20offline.%0A%20%20%20%20%20--,grace,-%3A%20A%20period%20in"))
Copy link
Contributor

Choose a reason for hiding this comment

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

I think a link the general page is fine. If there is an internal reference IE:
https://somepage.com#part2
Definitely include that. But I'd remove the URL encoding if it isn't necessary.

exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/DrillRoot.java Show resolved Hide resolved
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/DrillRoot.java Outdated Show resolved Hide resolved
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/DrillRoot.java Outdated Show resolved Hide resolved
exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/DrillRoot.java Outdated Show resolved Hide resolved
@estherbuchwalter
Copy link
Contributor Author

Thank you @cgivre for your review! I've reverted the code reformatting and fixed the URLs to point to the general documentation page. Also, I reformatted the log message (I didn't see any other examples of incorrectly formatted logs in these files).
I'm building Drill now and then will push the changes here.

cgivre
cgivre approved these changes Mar 11, 2022
View reviewed changes
Copy link
Contributor

@cgivre cgivre left a comment

Choose a reason for hiding this comment

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

@estherbuchwalter
Thanks for this. LGTM +1 (Pending CI)

@cgivre cgivre merged commit 569713f into apache:master Mar 11, 2022
jnturton pushed a commit to jnturton/drill that referenced this pull request Jul 11, 2022
@estherbuchwalter @jnturton
DRILL-8162: Add OpenAPI Specification documentation for Drill's REST …
ea83f48 
…API (apache#2489)

* Excluded swagger dependencies to fix enforcer error

* Added swagger core dependencies and swagger version

* Tested adding swagger annotations

* Added external docs in the openAPI Specification/swagger annotations

* Reverted code reformat. Deencoded URLs. Reformatted log messages.

* More format changes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cgivre cgivre cgivre approved these changes

Assignees

@estherbuchwalter estherbuchwalter

Labels
documentation PRs relating to documentation enhancement PRs that add a new functionality to Drill
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@estherbuchwalter @cgivre