[GitHub] [drill] cgivre commented on a change in pull request #2489: DRILL-8162: Add OpenAPI Specification documentation for Drill's REST API

[GitBox]

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 = 

[GitHub] [drill] cgivre commented on a change in pull request #2489: DRILL-8162: Add OpenAPI Specification documentation for Drill's REST API

[GitBox]

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 @@
   2
   false
   
${project.build.directory}/generated-sources/