[GitHub] [druid] paul-rogers commented on a diff in pull request #13052: Expose HTTP Response headers from SqlResource

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

paul-rogers commented on code in PR #13052:
URL: https://github.com/apache/druid/pull/13052#discussion_r967634250


##########
sql/src/main/java/org/apache/druid/sql/guice/SqlModule.java:
##########
@@ -130,4 +137,54 @@ private boolean isAvaticaEnabled()
     Preconditions.checkNotNull(props, "props");
     return Boolean.valueOf(props.getProperty(PROPERTY_SQL_ENABLE_AVATICA, "true"));
   }
+
+  /**
+   * We create a new class for this module so that it can be shared by tests.  The structuring of the SqlModule
+   * at time of writing was not conducive to reuse in test code, so, instead of fixing that we just take the easy
+   * way out of adding the test-reusable code to this module and reuse that.
+   */
+  public static class SqlStatementFactoryModule implements Module

Review Comment:
   Thanks for this. This is some fancy Guice wrangling that makes the setup a bit cleaner.



##########
server/src/main/java/org/apache/druid/server/QueryLifecycle.java:
##########
@@ -151,30 +151,40 @@ public <T> Sequence<T> runSimple(
 
     final Sequence<T> results;
 
+    final QueryResponse queryResponse;
     try {
       preAuthorized(authenticationResult, authorizationResult);
       if (!authorizationResult.isAllowed()) {
         throw new ISE("Unauthorized");
       }
 
-      final QueryResponse queryResponse = execute();
+      queryResponse = execute();
       results = queryResponse.getResults();
     }
     catch (Throwable e) {
       emitLogsAndMetrics(e, null, -1);
       throw e;
     }
 
-    return Sequences.wrap(
-        results,
-        new SequenceWrapper()
-        {
-          @Override
-          public void after(final boolean isDone, final Throwable thrown)
-          {
-            emitLogsAndMetrics(thrown, null, -1);
-          }
-        }
+    /*
+     * It seems extremely weird that the below code is wrapping the Sequence in order to emitLogsAndMetrics.
+     * The Sequence was returned by the call to execute, it would be worthwile to figure out why this wrapping
+     * cannot be moved into execute().  We leave this as an exercise for the future, however as this oddity
+     * was discovered while just trying to expose HTTP response headers

Review Comment:
   Native queries when running from SQL should probably not emit logs and metrics since there is code in the SQL layer to do that: assuming these are basically the same logs and metrics. Are we emitting SQL logs and metrics in SQL, but native ones here? Is that redundant? Should we figure this out?



##########
sql/src/main/java/org/apache/druid/sql/SqlStatementFactory.java:
##########
@@ -23,11 +23,30 @@
 
 import javax.servlet.http.HttpServletRequest;
 
-public interface SqlStatementFactory
+public class SqlStatementFactory
 {
-  HttpStatement httpStatement(SqlQuery sqlQuery, HttpServletRequest req);
+  private final SqlToolbox lifecycleToolbox;
 
-  DirectStatement directStatement(SqlQueryPlus sqlRequest);
+  public SqlStatementFactory(SqlToolbox lifecycleToolbox)

Review Comment:
   Perhaps add a comment to point the reader to the `SqlModule` and `SqlStatementFactoryModule` to explain how these instances are created. It is, unfortunately, not intuitively obvious on the first read how things are now wired up.



##########
sql/src/main/java/org/apache/druid/sql/http/SqlResource.java:
##########
@@ -87,40 +89,28 @@
   private final SqlStatementFactory sqlStatementFactory;
   private final SqlLifecycleManager sqlLifecycleManager;
   private final ServerConfig serverConfig;
+  private final ResponseContextConfig responseContextConfig;
+  private final DruidNode selfNode;
 
   @Inject
-  public SqlResource(
-      @Json ObjectMapper jsonMapper,
-      AuthorizerMapper authorizerMapper,
-      NativeSqlEngine engine,
-      SqlStatementFactoryFactory sqlStatementFactoryFactory,
-      SqlLifecycleManager sqlLifecycleManager,
-      ServerConfig serverConfig
-  )
-  {
-    this(
-        jsonMapper,
-        authorizerMapper,
-        sqlStatementFactoryFactory.factorize(engine),
-        sqlLifecycleManager,
-        serverConfig
-    );
-  }
-
-  @VisibleForTesting
   SqlResource(
       final ObjectMapper jsonMapper,
       final AuthorizerMapper authorizerMapper,
-      final SqlStatementFactory sqlStatementFactory,
+      final @NativeQuery SqlStatementFactory sqlStatementFactory,
       final SqlLifecycleManager sqlLifecycleManager,
-      final ServerConfig serverConfig
+      final ServerConfig serverConfig,
+      ResponseContextConfig responseContextConfig,
+      @Self DruidNode selfNode

Review Comment:
   Nit: `add final` to the above two items just to be consistent? I don't think the Java compiler cares: it can figure out that the items are essentially final. But, we seem to like including the statements as documentation.



##########
extensions-core/multi-stage-query/src/test/java/org/apache/druid/msq/test/MSQTestBase.java:
##########
@@ -341,7 +344,17 @@ public String getFormatString()
         new JoinableFactoryModule(),
         new IndexingServiceTuningConfigModule(),
         new MSQIndexingModule(),
-        new MSQSqlModule(),
+        Modules.override(new MSQSqlModule()).with(
+            binder -> {
+              // Our Guice configuration currently requires bindings to exist even if they aren't ever used, the
+              // following bindings are overriding other bindings that end up needing a lot more dependencies.
+              // We replace the bindings with something that returns null to make things more brittle in case they
+              // actually are used somewhere in the test.
+              binder.bind(SqlStatementFactory.class).annotatedWith(MSQ.class).toProvider(Providers.of(null));

Review Comment:
   This only works if the MSQ tests never use the SQL layer (at least via Guice.) Our Calcite tests are currently a muddle because they hand-build a set of objects, maintained in static variables. An open PR starts to fix this with the ultimate goal of using Guice to create the objects so we don't have to entirely different systems to maintain.
   
   In the branch where the Calcite test refactoring is happening, I ran into issues with the new MSQ tests: they do seem to use Guice, but with objects created in that static bundle of objects. Still trying to sort out that bit of confusion.
   
   The question is then, do the MSQ tests use hand-created objects for the SQL layer or objects from Guice? This change suggests that they are hand-created and the Guice-provided ones are ignored (or we have multiple Guice injectors in play.)
   
   Perhaps the best way to proceed is to commit this, then I'll rebase the Calcite test cleanup on this PR and see what's what. 



-- 
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: commits-unsubscribe@druid.apache.org

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


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org