kfaraz commented on code in PR #18424:
URL: https://github.com/apache/druid/pull/18424#discussion_r2333874435
##########
server/src/main/java/org/apache/druid/initialization/ServerInjectorBuilder.java:
##########
@@ -73,6 +73,23 @@ public static Injector makeServerInjector(
.build();
}
+ /**
+ * Rough bridge solution for Hadoop indexing that needs server-like Injector
but can't run jetty 12
Review Comment:
Instead of saying `Rough bridge solution all ...`, can we just say `Needed
for ...` in all the places.
##########
server/src/main/java/org/apache/druid/initialization/CoreInjectorBuilder.java:
##########
@@ -137,4 +137,52 @@ public CoreInjectorBuilder forServer()
);
return this;
}
+
+ /**
+ * Rough bridge solution for Hadoop indexing that needs server-like Injector
but can't run jetty 12
+ */
+ @Deprecated
+ public CoreInjectorBuilder forServerWithoutJetty()
+ {
+ withLogging();
+ withLifecycle();
+ add(
+ ExtensionsModule.SecondaryModule.class,
+ new DruidAuthModule(),
+ new PolicyModule(),
+ TLSCertificateCheckerModule.class,
+ EmitterModule.class,
+ HttpClientModule.global(),
+ HttpClientModule.escalatedGlobal(),
+ new HttpClientModule("druid.broker.http", Client.class, true),
+ new HttpClientModule("druid.broker.http", EscalatedClient.class, true),
+ new CuratorModule(),
+ new AnnouncerModule(),
+ new MetricsModule(),
+ new SegmentWriteOutMediumModule(),
+ new ServerModule(),
+ new StorageNodeModule(),
+ new ExpressionModule(),
+ new BuiltInTypesModule(),
+ new DiscoveryModule(),
+ new ServerViewModule(),
+ new MetadataConfigModule(),
+ new DerbyMetadataStorageDruidModule(),
+ new JacksonConfigManagerModule(),
+ new LocalDataStorageDruidModule(),
+ new TombstoneDataStorageModule(),
+ new JavaScriptModule(),
+ new AuthenticatorModule(),
+ new AuthenticatorMapperModule(),
+ new EscalatorModule(),
+ new AuthorizerModule(),
+ new AuthorizerMapperModule(),
+ new StartupLoggingModule(),
+ new ExternalStorageAccessSecurityModule(),
+ new ServiceClientModule(),
+ new StorageConnectorModule(),
+ new CatalogCoreModule()
Review Comment:
Rather than duplicating the whole list, let's just call the new method from
`forServer` and have an extra `add(new JettyServerModule())` in there.
##########
sql/src/main/java/org/apache/druid/sql/avatica/DruidAvaticaJsonHandler.java:
##########
@@ -19,46 +19,127 @@
package org.apache.druid.sql.avatica;
-import com.google.inject.Inject;
+import org.apache.calcite.avatica.AvaticaUtils;
+import org.apache.calcite.avatica.metrics.MetricsSystem;
+import org.apache.calcite.avatica.metrics.Timer;
+import org.apache.calcite.avatica.remote.JsonHandler;
import org.apache.calcite.avatica.remote.LocalService;
+import org.apache.calcite.avatica.remote.MetricsHelper;
import org.apache.calcite.avatica.remote.Service;
import org.apache.calcite.avatica.server.AvaticaJsonHandler;
+import org.apache.calcite.avatica.server.MetricsAwareAvaticaHandler;
+import org.apache.calcite.avatica.util.UnsynchronizedBuffer;
import org.apache.druid.guice.annotations.Self;
import org.apache.druid.java.util.common.StringUtils;
+import org.apache.druid.java.util.common.logger.Logger;
import org.apache.druid.server.DruidNode;
+import org.eclipse.jetty.io.Content;
+import org.eclipse.jetty.server.Handler;
import org.eclipse.jetty.server.Request;
+import org.eclipse.jetty.server.Response;
+import org.eclipse.jetty.util.Callback;
-import javax.servlet.ServletException;
-import javax.servlet.http.HttpServletRequest;
-import javax.servlet.http.HttpServletResponse;
-import java.io.IOException;
+import javax.inject.Inject;
+import java.io.InputStream;
+import java.nio.ByteBuffer;
+import java.nio.charset.StandardCharsets;
+import java.util.Objects;
-public class DruidAvaticaJsonHandler extends AvaticaJsonHandler
+public class DruidAvaticaJsonHandler extends Handler.Abstract implements
MetricsAwareAvaticaHandler
Review Comment:
I think it would be nice to add a `DruidAvaticaHandler` at this point which
can be extended by both the Json handler and the Protobuf handler. It was not
possible earlier since both implemented different interfaces.
We can put the common logic (if any) in the base `DruidAvaticaHandler` class.
Even if there is no common logic, it would still be more manageable in the
long term.
##########
server/src/main/java/org/apache/druid/initialization/ServerInjectorBuilder.java:
##########
@@ -118,6 +135,37 @@ public Injector build()
return new ExtensionInjectorBuilder(serviceBuilder).build();
}
+ /**
+ * Rough bridge solution for Hadoop indexing that needs server-like Injector
but can't run jetty 12
+ */
+ public Injector buildWithoutJettyModules()
Review Comment:
Let's try to common out the parts of this method with the existing `build`
method.
##########
sql/src/test/java/org/apache/druid/quidem/DruidAvaticaTestDriver.java:
##########
@@ -186,7 +186,7 @@ public void close()
}
}
- protected AbstractAvaticaHandler getAvaticaHandler(final DruidMeta
druidMeta)
+ protected Handler.Abstract getAvaticaHandler(final DruidMeta druidMeta)
Review Comment:
This could return the `DruidAvaticaHandler` suggested in the other comment.
##########
docs/configuration/index.md:
##########
@@ -1496,6 +1496,7 @@ Druid uses Jetty to serve HTTP requests.
|`druid.server.http.enableForwardedRequestCustomizer`|If enabled, adds Jetty
ForwardedRequestCustomizer which reads X-Forwarded-* request headers to
manipulate servlet request object when Druid is used behind a proxy.|false|
|`druid.server.http.allowedHttpMethods`|List of HTTP methods that should be
allowed in addition to the ones required by Druid APIs. Druid APIs require GET,
PUT, POST, and DELETE, which are always allowed. This option is not useful
unless you have installed an extension that needs these additional HTTP methods
or that adds functionality related to CORS. None of Druid's bundled extensions
require these methods.|`[]`|
|`druid.server.http.contentSecurityPolicy`|Content-Security-Policy header
value to set on each non-POST response. Setting this property to an empty
string, or omitting it, both result in the default `frame-ancestors: none`
being set.|`frame-ancestors 'none'`|
+|`druid.server.http.uriCompliance`|Jetty `UriCompliance` mode for Druid's
embedded Jetty servers. To modify, override this config with the string
representation of any `UriCompliance` mode that Jetty supports.|LEGACY|
Review Comment:
Can we also list out the possible values of this config?
Or link to the Jetty page which lists the values.
##########
server/src/main/java/org/apache/druid/initialization/CoreInjectorBuilder.java:
##########
@@ -137,4 +137,52 @@ public CoreInjectorBuilder forServer()
);
return this;
}
+
+ /**
+ * Rough bridge solution for Hadoop indexing that needs server-like Injector
but can't run jetty 12
+ */
+ @Deprecated
Review Comment:
I don't think this should be marked deprecated. It just provides a subset of
all the modules.
##########
server/src/main/java/org/apache/druid/initialization/Initialization.java:
##########
@@ -62,4 +62,16 @@ public static Injector makeInjectorWithModules(
{
return ServerInjectorBuilder.makeServerInjector(baseInjector,
ImmutableSet.of(), modules);
}
+
+ /**
+ * Rough bridge solution for Hadoop indexing that needs server-like Injector
but can't run jetty 12
+ */
+ @Deprecated
+ public static Injector makeInjectorWithoutJettyModules(
Review Comment:
The javadoc of this class says that we should use `StartupInjectorBuilder`
instead.
So, rather than adding a new deprecated method, should we try to use
`StartupInjectorBuilder` directly?
##########
docs/operations/java.md:
##########
@@ -27,7 +27,7 @@ a Java runtime for Druid.
## Selecting a Java runtime
- The project team recommends Java 17. Although you can use Java 11, support
for it is deprecated.
+Druid officially support Java 17.
Review Comment:
```suggestion
Druid officially supports Java 17.
```
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
