Expose HTTP Response headers from SqlResource by imply-cheddar · Pull Request #13052 · apache/druid

imply-cheddar · 2022-09-08T02:13:15Z

This change makes the SqlResource expose HTTP response headers in the same way that the QueryResource exposes them.

Fundamentally, the change is to pipe the QueryResponse object all the way through to the Resource so that it can populate response headers. There is also some code cleanup around DI, as there was a superfluous FactoryFactory class muddying things up.

This PR has:

been self-reviewed.
added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
added unit tests or modified existing tests to cover new code paths, ensuring the threshold for code coverage is met.

This change makes the SqlResource expose HTTP response headers in the same way that the QueryResource exposes them. Fundamentally, the change is to pipe the QueryResponse object all the way through to the Resource so that it can populate response headers. There is also some code cleanup around DI, as there was a superfluous FactoryFactory class muddying things up.

somu-imply · 2022-09-08T22:45:44Z

+      if (sqlQuery.includeHeader()) {
+        responseBuilder.header(SQL_HEADER_RESPONSE_HEADER, SQL_HEADER_VALUE);
      }
+


should this be always added or only in case where includeHeader() is true

That's code that previously existed and I didn't change. It just looks like I changed it because of diff stuff. Look at lines 191-193 of the original.

somu-imply · 2022-09-08T22:49:01Z

Overall looks good, the Sequence has been changed to a QueryResponse with a separate class and removing the old static QueryResponse class. Most of the places which deal with sequences now has an additional getResult as the return type of execute is now a QueryResponse object. Have one minor comment so far

clintropolis

seems like some legitimate failures

paul-rogers

@imply-cheddar, thanks for this improvement. A quick scan suggests the approach is sound. LGTM (non-binding)

Thanks for the cleanup of the SqlStatementFactory. I believe that awkward indirection was needed, in part, for some test hackery. If there is an issue, we'll fix it when it arises.

This change will conflict with an open PR to refactor the Calcite tests. However, I'll deal with that issue once this is committed.

Your approach to plumbing the QueryResponse up from the native engine parallels what I did in the "operator" branch for other reasons; the "operator" work can now build on your changes here.

paul-rogers · 2022-09-10T10:35:24Z

+              // 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));


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.

Yeah... when I was fixing up this test, it was definitely a bit of a mixed bag of strategies. What I saw is that some of the objects are coming from Guice, but some of the objects are being hand-created. I had to make this change because the place that I added the binding was used in the test, but the test also didn't have the other dependencies defined to make that "just work", so I had to give Guice just enough to let it make the injector

paul-rogers · 2022-09-10T10:37:46Z

+     * 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


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?

Which code in the SQL layer handles it? IIRC, the call on QueryLifecycle that this comment is on is special built only for the SQL code to use and I didn't see other code doing the logging, so I guessed that the code I commented on here is actually the only way that the SQL layer handles its logs and metrics.

We should figure it out, but not in this code change :). I already went sideways doing Guice stuff, don't want to go and expand scope even more ;).

paul-rogers · 2022-09-10T10:42:27Z

+  private final SqlToolbox lifecycleToolbox;

-  DirectStatement directStatement(SqlQueryPlus sqlRequest);
+  public SqlStatementFactory(SqlToolbox lifecycleToolbox)


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.

There is magic in DI platforms, to a certain degree, you must know the magic to know how things are wired up.

Generally speaking, there is one rule with using Guice that we should be following (I noticed that we have drifted from the rules in some of the code) to help know how things are wired up. Never, ever, ever, build on or expect implicit bindings (in the code I was looking at, I noticed a number of implicit bindings that were happening, I made explicit ones for everything that I found so far). We should always have an explicit binding in a module somewhere for everything that we expect to be bound. The reason for this is that without an explicit binding, it's impossible to do a usages search and figure out how things are setup. With an explicit binding, you can do a usages search on the class and then find the usages "as a .class" which tells you which Module it is bound in. Provider methods are kinda a gray area here and I generally try to keep away from them if possible. In this case, without revisiting the object structure in general, either a provider method or an actual provider would be required. For Provider methods, the .class usages don't show up, but if you also look for usages as a return value from a method, you can find the Provider methods as well. Additionally, usages of the constructor will show up here.

TBH, I would also just add the javadoc, but I'm scared of another multi-hour CI cycle blocking the PR. Maybe I'll do the javadoc and merge anyway.

paul-rogers · 2022-09-10T10:51:57Z

+   * 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


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

paul-rogers · 2022-09-10T10:53:54Z

-      final ServerConfig serverConfig
+      final ServerConfig serverConfig,
+      ResponseContextConfig responseContextConfig,
+      @Self DruidNode selfNode


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.

abhishekagarwal87

LGTM

cheddar · 2022-09-12T08:39:50Z

I pushed up javadoc fixes and travis got into sad-land from flakey tests :(. Going to merge anyway, it was a clean build ahead of the javadoc push.

* Expose HTTP Response headers from SqlResource This change makes the SqlResource expose HTTP response headers in the same way that the QueryResource exposes them. Fundamentally, the change is to pipe the QueryResponse object all the way through to the Resource so that it can populate response headers. There is also some code cleanup around DI, as there was a superfluous FactoryFactory class muddying things up.

imply-cheddar force-pushed the http-resp-headers-for-sql branch from 177b179 to 3eee4ce Compare September 8, 2022 02:14

imply-cheddar force-pushed the http-resp-headers-for-sql branch from 3eee4ce to fe0fcfa Compare September 8, 2022 04:09

somu-imply reviewed Sep 8, 2022

View reviewed changes

clintropolis reviewed Sep 8, 2022

View reviewed changes

imply-cheddar added 2 commits September 9, 2022 08:03

Fix MSQ Tests

546c0bb

Style and Review Feedback

276d8ec

clintropolis added the Area - SQL label Sep 9, 2022

paul-rogers reviewed Sep 10, 2022

View reviewed changes

Add requested javadoc

179b066

abhishekagarwal87 approved these changes Sep 12, 2022

View reviewed changes

abhishekagarwal87 added Refactoring Release Notes labels Sep 12, 2022

cheddar merged commit 5ba0075 into apache:master Sep 12, 2022

kfaraz added this to the 25.0 milestone Nov 22, 2022

This was referenced Dec 18, 2022

[Draft] 25.0.0 Release Notes #13592

Closed

Add SegmentAllocationQueue to batch allocation actions #13369

Merged

Conversation

imply-cheddar commented Sep 8, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

somu-imply commented Sep 8, 2022

Uh oh!

clintropolis left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

paul-rogers left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

imply-cheddar Sep 11, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

abhishekagarwal87 left a comment

Choose a reason for hiding this comment

Uh oh!

cheddar commented Sep 12, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

imply-cheddar Sep 11, 2022 •

edited

Loading