You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@drill.apache.org by GitBox <gi...@apache.org> on 2022/03/08 15:22:57 UTC
[GitHub] [drill] estherbuchwalter opened a new pull request #2489: DRILL-8162: Add OpenAPI Specification documentation for Drill's REST API
estherbuchwalter opened a new pull request #2489:
URL: https://github.com/apache/drill/pull/2489
# [DRILL-8162](https://issues.apache.org/jira/browse/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/](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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] cgivre commented on a change in pull request #2489: DRILL-8162: Add OpenAPI Specification documentation for Drill's REST API
Posted by GitBox <gi...@apache.org>.
cgivre commented on a change in pull request #2489:
URL: https://github.com/apache/drill/pull/2489#discussion_r823809822
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/DrillRoot.java
##########
@@ -181,19 +194,15 @@ public ClusterInfo getClusterInfoJSON() {
final String currentVersion = currentDrillbit.getVersion();
final DrillConfig config = dbContext.getConfig();
- final boolean userEncryptionEnabled =
- config.getBoolean(ExecConstants.USER_ENCRYPTION_SASL_ENABLED) ||
- config .getBoolean(ExecConstants.USER_SSL_ENABLED);
+ final boolean userEncryptionEnabled = config.getBoolean(ExecConstants.USER_ENCRYPTION_SASL_ENABLED) || config.getBoolean(ExecConstants.USER_SSL_ENABLED);
final boolean bitEncryptionEnabled = config.getBoolean(ExecConstants.BIT_ENCRYPTION_SASL_ENABLED);
OptionManager optionManager = work.getContext().getOptionManager();
final boolean isUserLoggedIn = AuthDynamicFeature.isUserLoggedIn(sc);
- final boolean shouldShowAdminInfo = isUserLoggedIn && ((DrillUserPrincipal)sc.getUserPrincipal()).isAdminUser();
+ final boolean shouldShowAdminInfo = isUserLoggedIn && ((DrillUserPrincipal) sc.getUserPrincipal()).isAdminUser();
Review comment:
nit: remove space.
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/DrillRoot.java
##########
@@ -181,19 +194,15 @@ public ClusterInfo getClusterInfoJSON() {
final String currentVersion = currentDrillbit.getVersion();
final DrillConfig config = dbContext.getConfig();
- final boolean userEncryptionEnabled =
- config.getBoolean(ExecConstants.USER_ENCRYPTION_SASL_ENABLED) ||
- config .getBoolean(ExecConstants.USER_SSL_ENABLED);
+ final boolean userEncryptionEnabled = config.getBoolean(ExecConstants.USER_ENCRYPTION_SASL_ENABLED) || config.getBoolean(ExecConstants.USER_SSL_ENABLED);
final boolean bitEncryptionEnabled = config.getBoolean(ExecConstants.BIT_ENCRYPTION_SASL_ENABLED);
OptionManager optionManager = work.getContext().getOptionManager();
final boolean isUserLoggedIn = AuthDynamicFeature.isUserLoggedIn(sc);
- final boolean shouldShowAdminInfo = isUserLoggedIn && ((DrillUserPrincipal)sc.getUserPrincipal()).isAdminUser();
+ final boolean shouldShowAdminInfo = isUserLoggedIn && ((DrillUserPrincipal) sc.getUserPrincipal()).isAdminUser();
for (DrillbitEndpoint endpoint : work.getContext().getAvailableBits()) {
- final DrillbitInfo drillbit = new DrillbitInfo(endpoint,
- isDrillbitsTheSame(currentDrillbit, endpoint),
- currentVersion.equals(endpoint.getVersion()));
+ final DrillbitInfo drillbit = new DrillbitInfo(endpoint, isDrillbitsTheSame(currentDrillbit, endpoint), currentVersion.equals(endpoint.getVersion()));
Review comment:
Nit: Please revert spacing changes. Here and elsewhere.
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/DrillRoot.java
##########
@@ -208,18 +217,12 @@ public ClusterInfo getClusterInfoJSON() {
String adminUsers = ExecConstants.ADMIN_USERS_VALIDATOR.getAdminUsers(optionManager);
String adminUserGroups = ExecConstants.ADMIN_USER_GROUPS_VALIDATOR.getAdminUserGroups(optionManager);
- logger.debug("Admin info: user: " + adminUsers + " user group: " + adminUserGroups +
- " userLoggedIn " + isUserLoggedIn + " shouldShowAdminInfo: " + shouldShowAdminInfo);
+ logger.debug("Admin info: user: " + adminUsers + " user group: " + adminUserGroups + " userLoggedIn " + isUserLoggedIn + " shouldShowAdminInfo: " + shouldShowAdminInfo);
Review comment:
Please format log messages as shown below: (I didn't do the whole line but you get the idea.)
```
logger.debug("Admin info: {} user group: {}", adminUsers, adminUserGroups);
```
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/DrillRoot.java
##########
@@ -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"))
Review comment:
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.
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/DrillRoot.java
##########
@@ -208,18 +217,12 @@ public ClusterInfo getClusterInfoJSON() {
String adminUsers = ExecConstants.ADMIN_USERS_VALIDATOR.getAdminUsers(optionManager);
String adminUserGroups = ExecConstants.ADMIN_USER_GROUPS_VALIDATOR.getAdminUserGroups(optionManager);
- logger.debug("Admin info: user: " + adminUsers + " user group: " + adminUserGroups +
- " userLoggedIn " + isUserLoggedIn + " shouldShowAdminInfo: " + shouldShowAdminInfo);
+ logger.debug("Admin info: user: " + adminUsers + " user group: " + adminUserGroups + " userLoggedIn " + isUserLoggedIn + " shouldShowAdminInfo: " + shouldShowAdminInfo);
- return new ClusterInfo(drillbits, currentVersion, mismatchedVersions,
- userEncryptionEnabled, bitEncryptionEnabled, shouldShowAdminInfo,
- QueueInfo.build(dbContext.getResourceManager()),
- processUser, processUserGroups, adminUsers, adminUserGroups, authEnabled.get());
+ return new ClusterInfo(drillbits, currentVersion, mismatchedVersions, userEncryptionEnabled, bitEncryptionEnabled, shouldShowAdminInfo, QueueInfo.build(dbContext.getResourceManager()), processUser, processUserGroups, adminUsers, adminUserGroups, authEnabled.get());
Review comment:
Please revert spacing changes, here and elsewheree.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] estherbuchwalter commented on a change in pull request #2489: DRILL-8162: Add OpenAPI Specification documentation for Drill's REST API
Posted by GitBox <gi...@apache.org>.
estherbuchwalter commented on a change in pull request #2489:
URL: https://github.com/apache/drill/pull/2489#discussion_r822783008
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/DrillRoot.java
##########
@@ -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"))
Review comment:
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?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] cgivre commented on a change in pull request #2489: DRILL-8162: Add OpenAPI Specification documentation for Drill's REST API
Posted by GitBox <gi...@apache.org>.
cgivre commented on a change in pull request #2489:
URL: https://github.com/apache/drill/pull/2489#discussion_r822198917
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/DrillRestServer.java
##########
@@ -188,20 +198,13 @@ public WebUserConnection provide() {
}
// User is logged in, get/set the WebSessionResources attribute
- WebSessionResources webSessionResources =
- (WebSessionResources) session.getAttribute(WebSessionResources.class.getSimpleName());
+ WebSessionResources webSessionResources = (WebSessionResources) session.getAttribute(WebSessionResources.class.getSimpleName());
if (webSessionResources == null) {
// User is login in for the first time
final DrillbitContext drillbitContext = workManager.getContext();
final DrillConfig config = drillbitContext.getConfig();
- final UserSession drillUserSession = UserSession.Builder.newBuilder()
- .withCredentials(UserBitShared.UserCredentials.newBuilder()
- .setUserName(sessionUserPrincipal.getName())
- .build())
- .withOptionManager(drillbitContext.getOptionManager())
- .setSupportComplexTypes(config.getBoolean(ExecConstants.CLIENT_SUPPORT_COMPLEX_TYPES))
- .build();
+ final UserSession drillUserSession = UserSession.Builder.newBuilder().withCredentials(UserBitShared.UserCredentials.newBuilder().setUserName(sessionUserPrincipal.getName()).build()).withOptionManager(drillbitContext.getOptionManager()).setSupportComplexTypes(config.getBoolean(ExecConstants.CLIENT_SUPPORT_COMPLEX_TYPES)).build();
Review comment:
nit: Please revert the line breaks to the original pattern for readability, here and elsewhere.
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/DrillRestServer.java
##########
@@ -73,6 +78,11 @@
import java.util.ArrayList;
import java.util.List;
+@OpenAPIDefinition(info = @Info(title = "Apache Drill REST API", description = "OpenAPI Specification", license = @License(name = "Apache Software Foundation (ASF)", url = "http://www.apache" + ".org/licenses/LICENSE-2.0"), contact = @Contact(name = "Apache Drill", url = "https://drill.apache.org/")))/*,
+tags = {
+ @Tag(name = "My Tag 1", description = "Tag 1's Description", externalDocs = @ExternalDocumentation(description = "docs description1")),
+ @Tag(name = "My Tag 2", description = "Tag 2's Description", externalDocs = @ExternalDocumentation(description = "docs description2"))})*/
Review comment:
Please remove commented out code, here and elsewhere. The line above, please break that up a bit so that it fits w/o horizontal scroll.
##########
File path: exec/java-exec/pom.xml
##########
@@ -889,11 +914,11 @@
<lookAhead>2</lookAhead>
<isStatic>false</isStatic>
<outputDirectory>${project.build.directory}/generated-sources/</outputDirectory>
-<!--
Review comment:
Do you know why we have this commented out code in this `pom.xml` file? I'd imagine it is here for a reason, and if so, we could indicate that. If you have no idea, it's fine and we can just leave it.
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/DrillRoot.java
##########
@@ -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"))
Review comment:
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.
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/DrillRoot.java
##########
@@ -230,278 +240,300 @@ public ClusterInfo getClusterInfoJSON() {
* @return true if drillbit are the same
*/
private boolean isDrillbitsTheSame(DrillbitEndpoint endpoint1, DrillbitEndpoint endpoint2) {
- return endpoint1.getAddress().equals(endpoint2.getAddress()) &&
- endpoint1.getControlPort() == endpoint2.getControlPort() &&
- endpoint1.getDataPort() == endpoint2.getDataPort() &&
- endpoint1.getUserPort() == endpoint2.getUserPort();
+ return endpoint1.getAddress().equals(endpoint2.getAddress()) && endpoint1.getControlPort() == endpoint2.getControlPort() && endpoint1.getDataPort() == endpoint2.getDataPort() && endpoint1.getUserPort() == endpoint2.getUserPort();
}
private Response setResponse(Map<String, ?> entity) {
- return Response.ok()
- .entity(entity)
- .header("Access-Control-Allow-Origin", "*")
- .header("Access-Control-Allow-Methods", "GET, POST, DELETE, PUT")
- .header("Access-Control-Allow-Credentials","true")
- .allow("OPTIONS").build();
+ return Response.ok().entity(entity).header("Access-Control-Allow-Origin", "*").header("Access-Control-Allow-Methods", "GET, POST, DELETE, PUT").header("Access-Control-Allow-Credentials", "true").allow("OPTIONS").build();
}
private Response shutdown(String resp) throws Exception {
Map<String, String> shutdownInfo = new HashMap<String, String>();
new Thread(new Runnable() {
- @Override
- public void run() {
- try {
- drillbit.close();
- } catch (Exception e) {
- logger.error("Request to shutdown drillbit failed", e);
- }
+ @Override
+ public void run() {
+ try {
+ drillbit.close();
+ } catch (Exception e) {
+ logger.error("Request to shutdown drillbit failed", e);
}
- }).start();
- shutdownInfo.put("response",resp);
+ }
+ }).start();
+ shutdownInfo.put("response", resp);
return setResponse(shutdownInfo);
}
-/**
- * Pretty-printing wrapper class around the ZK-based queue summary.
- */
+ /**
+ * Pretty-printing wrapper class around the ZK-based queue summary.
+ */
-@XmlRootElement
-public static class QueueInfo {
- private final ZKQueueInfo zkQueueInfo;
+ @XmlRootElement
+ public static class QueueInfo {
+ private final ZKQueueInfo zkQueueInfo;
- public static QueueInfo build(ResourceManager rm) {
+ public static QueueInfo build(ResourceManager rm) {
- // Consider queues enabled only if the ZK-based queues are in use.
+ // Consider queues enabled only if the ZK-based queues are in use.
- ThrottledResourceManager throttledRM = null;
- if (rm != null && rm instanceof DynamicResourceManager) {
- DynamicResourceManager dynamicRM = (DynamicResourceManager) rm;
- rm = dynamicRM.activeRM();
+ ThrottledResourceManager throttledRM = null;
+ if (rm != null && rm instanceof DynamicResourceManager) {
Review comment:
Why are these changes here? This looks like this came from another branch or PR. Can you please rebase on current master and push again?
```
> git checkout master
> git fetch upstream
> git rebase upstream/master
> git checkout <your_branch>
> git rebase master
> git push -f
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] estherbuchwalter commented on pull request #2489: DRILL-8162: Add OpenAPI Specification documentation for Drill's REST API
Posted by GitBox <gi...@apache.org>.
estherbuchwalter commented on pull request #2489:
URL: https://github.com/apache/drill/pull/2489#issuecomment-1064288043
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] cgivre merged pull request #2489: DRILL-8162: Add OpenAPI Specification documentation for Drill's REST API
Posted by GitBox <gi...@apache.org>.
cgivre merged pull request #2489:
URL: https://github.com/apache/drill/pull/2489
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] cgivre edited a comment on pull request #2489: DRILL-8162: Add OpenAPI Specification documentation for Drill's REST API
Posted by GitBox <gi...@apache.org>.
cgivre edited a comment on pull request #2489:
URL: https://github.com/apache/drill/pull/2489#issuecomment-1062404994
@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!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] cgivre commented on pull request #2489: DRILL-8162: Add OpenAPI Specification documentation for Drill's REST API
Posted by GitBox <gi...@apache.org>.
cgivre commented on pull request #2489:
URL: https://github.com/apache/drill/pull/2489#issuecomment-1062404994
@estherbuchwalter Can you please rebase on current master?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] estherbuchwalter commented on a change in pull request #2489: DRILL-8162: Add OpenAPI Specification documentation for Drill's REST API
Posted by GitBox <gi...@apache.org>.
estherbuchwalter commented on a change in pull request #2489:
URL: https://github.com/apache/drill/pull/2489#discussion_r822706862
##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/DrillRestServer.java
##########
@@ -188,20 +198,13 @@ public WebUserConnection provide() {
}
// User is logged in, get/set the WebSessionResources attribute
- WebSessionResources webSessionResources =
- (WebSessionResources) session.getAttribute(WebSessionResources.class.getSimpleName());
+ WebSessionResources webSessionResources = (WebSessionResources) session.getAttribute(WebSessionResources.class.getSimpleName());
if (webSessionResources == null) {
// User is login in for the first time
final DrillbitContext drillbitContext = workManager.getContext();
final DrillConfig config = drillbitContext.getConfig();
- final UserSession drillUserSession = UserSession.Builder.newBuilder()
- .withCredentials(UserBitShared.UserCredentials.newBuilder()
- .setUserName(sessionUserPrincipal.getName())
- .build())
- .withOptionManager(drillbitContext.getOptionManager())
- .setSupportComplexTypes(config.getBoolean(ExecConstants.CLIENT_SUPPORT_COMPLEX_TYPES))
- .build();
+ final UserSession drillUserSession = UserSession.Builder.newBuilder().withCredentials(UserBitShared.UserCredentials.newBuilder().setUserName(sessionUserPrincipal.getName()).build()).withOptionManager(drillbitContext.getOptionManager()).setSupportComplexTypes(config.getBoolean(ExecConstants.CLIENT_SUPPORT_COMPLEX_TYPES)).build();
Review comment:
Ok, sure. I think this is from reformatting the code.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [drill] estherbuchwalter commented on a change in pull request #2489: DRILL-8162: Add OpenAPI Specification documentation for Drill's REST API
Posted by GitBox <gi...@apache.org>.
estherbuchwalter commented on a change in pull request #2489:
URL: https://github.com/apache/drill/pull/2489#discussion_r822700830
##########
File path: exec/java-exec/pom.xml
##########
@@ -889,11 +914,11 @@
<lookAhead>2</lookAhead>
<isStatic>false</isStatic>
<outputDirectory>${project.build.directory}/generated-sources/</outputDirectory>
-<!--
Review comment:
No, I don't know what this code is for.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: dev-unsubscribe@drill.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org