[GitHub] [drill] paul-rogers commented on a change in pull request #2194: DRILL-7888: query.json returns an incorrect error message when the query fails

[GitBox <gi...@apache.org>]

paul-rogers commented on a change in pull request #2194:
URL: https://github.com/apache/drill/pull/2194#discussion_r605407739



##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/server/options/SystemOptionManager.java
##########
@@ -257,6 +257,7 @@
       new OptionDefinition(ClassCompilerSelector.JAVA_COMPILER_JANINO_MAXSIZE),
       new OptionDefinition(ClassCompilerSelector.JAVA_COMPILER_DEBUG),
       new OptionDefinition(ExecConstants.ENABLE_VERBOSE_ERRORS),
+      new OptionDefinition(ExecConstants.ENABLE_REST_VERBOSE_ERRORS),

Review comment:
       There is one more step: add the default value to the `exec` version of `drill-module.conf`.

##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/BaseQueryRunner.java
##########
@@ -45,7 +45,7 @@
 
   protected final WorkManager workManager;
   protected final WebUserConnection webUserConnection;
-  private final OptionSet options;
+  protected final OptionSet options;

Review comment:
       Probably not needed. See the `applyOptions` method and below.

##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/ExecConstants.java
##########
@@ -1216,6 +1216,11 @@ public static String bootDefaultFor(String name) {
           "the sender to send out its data more rapidly, but you should know that it has a risk to OOM when the system is solving parallel " +
           "large queries until we have a more accurate resource manager."));
 
+
+  public static final String ENABLE_REST_VERBOSE_ERRORS_KEY = "rest.errors.verbose";

Review comment:
       The name here should be consistent with other similar keys. For example:
   
   ```
     public static final String HTTP_ENABLE = "drill.exec.http.enabled";
   ```
   
   So, maybe, `drill.exec.http.rest.errors.verbose`. Kind of ugly.
   
   We had talked about reworking the keys now that we know what they all are. But, until that is done as a unified task, best to maintain the existing naming conventions so we don't make things more of a mess than they already are.

##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/stream/QueryRunner.java
##########
@@ -48,7 +48,7 @@ public void start(QueryWrapper query) throws ValidationException {
     startQuery(QueryType.valueOf(query.getQueryType()),
         query.getQuery(),
         userConn);
-    userConn.onStart(queryId, maxRows);
+    userConn.onStart(queryId, maxRows, options);

Review comment:
       Not needed. The connection already has this member:
   
   ```
     protected WebSessionResources webSessionResources;
   ```
   
   Which provides:
   
   ```
     private final UserSession webUserSession;
   ```
   
   Which provides `getOptions()`

##########
File path: exec/java-exec/src/main/java/org/apache/drill/exec/server/rest/stream/StreamingHttpConnection.java
##########
@@ -80,9 +85,10 @@ public StreamingHttpConnection(WebSessionResources webSessionResources) {
    * Provide query info once the query starts. Sent from the REST request
    * thread.
    */
-  public void onStart(QueryId queryId, int rowLimit) {
+  public void onStart(QueryId queryId, int rowLimit, OptionSet options) {

Review comment:
       Not needed. See 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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org