kfaraz commented on code in PR #18424:
URL: https://github.com/apache/druid/pull/18424#discussion_r2357937876
##########
server/src/main/java/org/apache/druid/initialization/ServerInjectorBuilder.java:
##########
@@ -73,6 +73,22 @@ public static Injector makeServerInjector(
.build();
}
+ /**
+ * Needed for Hadoop indexing that needs server-like Injector but can't run
jetty 12
+ */
+ @VisibleForTesting
Review Comment:
I think this method is used by `HadoopDruidIndexerConfig` too. If that is
the case, let's remove this annotation.
##########
server/src/main/java/org/apache/druid/initialization/ServerInjectorBuilder.java:
##########
@@ -91,6 +107,33 @@ public ServerInjectorBuilder serviceModules(final
Iterable<? extends Module> mod
}
public Injector build()
+ {
+ return this.build(true);
+ }
+
+ /**
+ * Needed for Hadoop indexing that needs server-like Injector but can't run
jetty 12
+ */
+ public Injector buildWithoutJettyModules()
+ {
+ return this.build(false);
+ }
+
+ public static Module registerNodeRoleModule(Set<NodeRole> nodeRoles)
Review Comment:
It seems that this method is unchanged. Can we move it back to its original
position so that we can avoid it from showing up in the diff?
##########
integration-tests/stop_cluster.sh:
##########
@@ -14,6 +14,15 @@
# See the License for the specific language governing permissions and
# limitations under the License.
+# Determine if docker compose is available. If not, assume Docker supports
Review Comment:
Does this change need to be included in this PR? Or can it be done
separately too?
##########
server/src/test/java/org/apache/druid/initialization/ServerConfigTest.java:
##########
@@ -79,6 +81,7 @@ public void testSerde() throws Exception
Assert.assertEquals("my-cool-policy",
modifiedConfig.getContentSecurityPolicy());
Assert.assertEquals("my-cool-policy",
modifiedConfig2.getContentSecurityPolicy());
Assert.assertTrue(modifiedConfig2.isEnableHSTS());
+ Assert.assertEquals(UriCompliance.RFC3986.getName(),
modifiedConfig2.getUriCompliance());
Review Comment:
Please update one of the tests to verify the default value of this config
too.
##########
pom.xml:
##########
@@ -754,26 +754,44 @@
<groupId>org.eclipse.jetty</groupId>
<artifactId>jetty-server</artifactId>
<version>${jetty.version}</version>
+ <exclusions>
+ <exclusion>
+ <groupId>org.eclipse.jetty.toolchain</groupId>
+ <artifactId>jetty-servlet-api</artifactId>
+ </exclusion>
+ </exclusions>
</dependency>
<dependency>
- <groupId>org.eclipse.jetty</groupId>
- <artifactId>jetty-servlet</artifactId>
+ <groupId>org.eclipse.jetty.ee8</groupId>
+ <artifactId>jetty-ee8-servlet</artifactId>
<version>${jetty.version}</version>
+ <exclusions>
Review Comment:
Nit: indentation seems off here.
##########
server/src/main/java/org/apache/druid/server/initialization/jetty/StandardResponseHeaderFilterHolder.java:
##########
@@ -66,16 +66,17 @@ public StandardResponseHeaderFilterHolder(final
ServerConfig serverConfig)
* Remove any standard headers in proxyResponse if they were also set in the
origin response, serverResponse.
* This prevents duplicates headers from appearing in proxy responses.
*
- * Used by implementations of {@link
org.eclipse.jetty.proxy.AsyncProxyServlet}.
+ * Used by implementations of {@link
org.eclipse.jetty.ee8.proxy.AsyncProxyServlet}.
*/
public static void deduplicateHeadersInProxyServlet(
final HttpServletResponse proxyResponse,
final Response serverResponse
)
{
for (final String headerName :
StandardResponseHeaderFilterHolder.STANDARD_HEADERS) {
- if (serverResponse.getHeaders().containsKey(headerName) &&
proxyResponse.containsHeader(headerName)) {
- ((org.eclipse.jetty.server.Response)
proxyResponse).getHttpFields().remove(headerName);
+ if (serverResponse.getHeaders().contains(headerName) &&
proxyResponse.containsHeader(headerName)) {
+ // In Jetty 12 EE8, use the standard servlet API method to remove
headers
Review Comment:
By "standard servlet API method", are you referring to
`proxyResponse.setHeader()` or some other method that we would like to use but
are not using here?
##########
server/src/test/java/org/apache/druid/server/initialization/jetty/StandardResponseHeaderFilterHolderTest.java:
##########
@@ -130,6 +139,40 @@ public void test_get_invalidContentSecurityPolicy()
);
}
+ @Test
+ public void test_deduplicateHeadersInProxyServlet_duplicatesExist()
+ {
+
EasyMock.expect(proxyResponse.containsHeader("Cache-Control")).andReturn(true).once();
+ proxyResponse.setHeader("Cache-Control", null);
+ EasyMock.expectLastCall().once();
+
EasyMock.expect(proxyResponse.containsHeader("Strict-Transport-Security")).andReturn(false).once();
+
+ EasyMock.expect(clientResponse.getHeaders()).andReturn(HttpFields.from(new
HttpField("Cache-Control", "true"), new HttpField("Strict-Transport-Security",
"true"))).times(3);
Review Comment:
Please break this up into multiple lines in any way you see fit.
##########
server/src/test/java/org/apache/druid/server/initialization/jetty/StandardResponseHeaderFilterHolderTest.java:
##########
@@ -130,6 +139,40 @@ public void test_get_invalidContentSecurityPolicy()
);
}
+ @Test
+ public void test_deduplicateHeadersInProxyServlet_duplicatesExist()
+ {
+
EasyMock.expect(proxyResponse.containsHeader("Cache-Control")).andReturn(true).once();
+ proxyResponse.setHeader("Cache-Control", null);
+ EasyMock.expectLastCall().once();
+
EasyMock.expect(proxyResponse.containsHeader("Strict-Transport-Security")).andReturn(false).once();
+
+ EasyMock.expect(clientResponse.getHeaders()).andReturn(HttpFields.from(new
HttpField("Cache-Control", "true"), new HttpField("Strict-Transport-Security",
"true"))).times(3);
+
+
Review Comment:
Nit: extra newline
##########
sql/src/main/java/org/apache/druid/sql/avatica/DruidAvaticaProtobufHandler.java:
##########
@@ -19,46 +19,100 @@
package org.apache.druid.sql.avatica;
-import com.google.inject.Inject;
-import org.apache.calcite.avatica.remote.LocalService;
+import org.apache.calcite.avatica.AvaticaUtils;
+import org.apache.calcite.avatica.metrics.Timer;
+import org.apache.calcite.avatica.remote.ProtobufHandler;
+import org.apache.calcite.avatica.remote.ProtobufTranslation;
+import org.apache.calcite.avatica.remote.ProtobufTranslationImpl;
import org.apache.calcite.avatica.remote.Service;
import org.apache.calcite.avatica.server.AvaticaProtobufHandler;
+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.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;
-public class DruidAvaticaProtobufHandler extends AvaticaProtobufHandler
+public class DruidAvaticaProtobufHandler extends DruidAvaticaHandler
{
+
+ private static final Logger LOG = new
Logger(DruidAvaticaProtobufHandler.class);
+
public static final String AVATICA_PATH_NO_TRAILING_SLASH =
"/druid/v2/sql/avatica-protobuf";
public static final String AVATICA_PATH = AVATICA_PATH_NO_TRAILING_SLASH +
"/";
+ private final ProtobufHandler pbHandler;
Review Comment:
```suggestion
private final ProtobufHandler protobufHandler;
```
##########
sql/src/test/java/org/apache/druid/sql/avatica/DruidAvaticaProtobufHandlerTest.java:
##########
@@ -35,12 +44,49 @@ protected String getJdbcUrlTail()
}
@Override
- protected AbstractAvaticaHandler getAvaticaHandler(final DruidMeta druidMeta)
+ protected Handler.Abstract getAvaticaHandler(final DruidMeta druidMeta)
Review Comment:
```suggestion
protected DruidAvaticaHandler getAvaticaHandler(final DruidMeta druidMeta)
```
##########
server/src/test/java/org/apache/druid/server/mocks/MockHttpServletResponse.java:
##########
@@ -337,4 +345,17 @@ public Locale getLocale()
{
throw new UnsupportedOperationException();
}
+
+
Review Comment:
Nit: extra newline.
##########
server/src/test/java/org/apache/druid/server/initialization/jetty/StandardResponseHeaderFilterHolderTest.java:
##########
@@ -130,6 +139,40 @@ public void test_get_invalidContentSecurityPolicy()
);
}
+ @Test
+ public void test_deduplicateHeadersInProxyServlet_duplicatesExist()
Review Comment:
```suggestion
public void test_deduplicateHeadersInProxyServlet_withDuplicates()
```
##########
indexing-hadoop/src/main/java/org/apache/druid/indexer/JobHelper.java:
##########
@@ -155,7 +155,8 @@ public static void setupClasspath(
for (String jarFilePath : jarFiles) {
final File jarFile = new File(jarFilePath);
- if (jarFile.getName().endsWith(".jar")) {
+ // Keep Druid jetty jars out of the classpath. They are not runnable in
the < 17 hadoop java runtime
+ if (jarFile.getName().endsWith(".jar") &&
!jarFile.getName().contains("jetty")) {
Review Comment:
Could we use `jetty-ee8-*` instead?
##########
server/src/main/java/org/apache/druid/server/initialization/ServerConfig.java:
##########
@@ -185,6 +188,9 @@ public ServerConfig(boolean enableQueryRequestsQueuing)
@JsonProperty
private boolean enableHSTS = false;
+ @JsonProperty
+ private String uriCompliance = UriCompliance.LEGACY.getName();
Review Comment:
Would it make sense to have this be of type `UriCompliance` rather than
`String`?
##########
sql/src/main/java/org/apache/druid/sql/avatica/DruidAvaticaHandler.java:
##########
@@ -0,0 +1,63 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.druid.sql.avatica;
+
+import org.apache.calcite.avatica.metrics.MetricsSystem;
+import org.apache.calcite.avatica.metrics.Timer;
+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.MetricsAwareAvaticaHandler;
+import org.apache.calcite.avatica.util.UnsynchronizedBuffer;
+import org.eclipse.jetty.server.Handler;
+
+public abstract class DruidAvaticaHandler extends Handler.Abstract implements
MetricsAwareAvaticaHandler
Review Comment:
Please add a short javadoc, if possible.
##########
server/src/main/java/org/apache/druid/server/initialization/jetty/JettyServerModule.java:
##########
@@ -467,25 +468,21 @@ public void stop()
server.setErrorHandler(new ErrorHandler()
{
@Override
- public boolean isShowServlet()
- {
- return false;
- }
-
- @Override
- public void handle(
- String target,
+ public boolean handle(
Request baseRequest,
- HttpServletRequest request,
- HttpServletResponse response
- ) throws IOException, ServletException
+ Response response,
+ Callback callback
+ ) throws Exception
{
- request.setAttribute(RequestDispatcher.ERROR_EXCEPTION, null);
- super.handle(target, baseRequest, request, response);
+ baseRequest.setAttribute(RequestDispatcher.ERROR_EXCEPTION, null);
+ return super.handle(baseRequest, response, callback);
}
});
}
+ log.info("Configuraing Jetty Request log for server");
Review Comment:
Is this needed?
##########
server/src/main/java/org/apache/druid/server/initialization/ServerConfig.java:
##########
@@ -306,6 +312,11 @@ public boolean isEnableQueryRequestsQueuing()
return enableQueryRequestsQueuing;
}
+ public String getUriCompliance()
Review Comment:
This should probably return a `UriCompliance` object rather than a String.
##########
sql/src/main/java/org/apache/druid/sql/avatica/DruidAvaticaProtobufHandler.java:
##########
@@ -19,46 +19,100 @@
package org.apache.druid.sql.avatica;
-import com.google.inject.Inject;
-import org.apache.calcite.avatica.remote.LocalService;
+import org.apache.calcite.avatica.AvaticaUtils;
+import org.apache.calcite.avatica.metrics.Timer;
+import org.apache.calcite.avatica.remote.ProtobufHandler;
+import org.apache.calcite.avatica.remote.ProtobufTranslation;
+import org.apache.calcite.avatica.remote.ProtobufTranslationImpl;
import org.apache.calcite.avatica.remote.Service;
import org.apache.calcite.avatica.server.AvaticaProtobufHandler;
+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.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;
-public class DruidAvaticaProtobufHandler extends AvaticaProtobufHandler
+public class DruidAvaticaProtobufHandler extends DruidAvaticaHandler
{
+
+ private static final Logger LOG = new
Logger(DruidAvaticaProtobufHandler.class);
+
public static final String AVATICA_PATH_NO_TRAILING_SLASH =
"/druid/v2/sql/avatica-protobuf";
public static final String AVATICA_PATH = AVATICA_PATH_NO_TRAILING_SLASH +
"/";
+ private final ProtobufHandler pbHandler;
+
@Inject
public DruidAvaticaProtobufHandler(
final DruidMeta druidMeta,
@Self final DruidNode druidNode,
- final AvaticaMonitor avaticaMonitor
+ final AvaticaMonitor metrics
)
{
- super(new LocalService(druidMeta), avaticaMonitor);
+ super(druidMeta, metrics, AvaticaProtobufHandler.class);
+ ProtobufTranslation protobufTranslation = new ProtobufTranslationImpl();
+ this.pbHandler = new ProtobufHandler(service, protobufTranslation,
this.metrics);
setServerRpcMetadata(new
Service.RpcMetadataResponse(druidNode.getHostAndPortToUse()));
}
@Override
- public void handle(
- final String target,
- final Request baseRequest,
- final HttpServletRequest request,
- final HttpServletResponse response
- ) throws IOException, ServletException
+ public boolean handle(Request request, Response response, Callback callback)
throws Exception
+ {
+ String requestURI = request.getHttpURI().getPath();
+ if
(AVATICA_PATH_NO_TRAILING_SLASH.equals(StringUtils.maybeRemoveTrailingSlash(requestURI)))
{
+ try (Timer.Context ctx = this.requestTimer.start()) {
+ if (!"POST".equals(request.getMethod())) {
+ response.setStatus(405);
+ response.write(
+ true,
+ ByteBuffer.wrap("This server expects only POST
calls.".getBytes(StandardCharsets.UTF_8)), callback
+ );
+ return true;
+ }
+ final byte[] requestBytes;
+ // Avoid a new buffer creation for every HTTP request
+ final UnsynchronizedBuffer buffer = threadLocalBuffer.get();
+ try (InputStream inputStream = Content.Source.asInputStream(request)) {
+ requestBytes = AvaticaUtils.readFullyToBytes(inputStream, buffer);
+ }
+ finally {
+ buffer.reset();
+ }
+
+ response.getHeaders().put("Content-Type",
"application/octet-stream;charset=utf-8");
+
+ org.apache.calcite.avatica.remote.Handler.HandlerResponse<byte[]>
handlerResponse;
+ try {
+ handlerResponse = pbHandler.apply(requestBytes);
+ }
+ catch (Exception e) {
+ LOG.debug(e, "Error invoking request");
+ handlerResponse = pbHandler.convertToErrorResponse(e);
+ }
+
+ response.setStatus(handlerResponse.getStatusCode());
+ response.write(true, ByteBuffer.wrap(handlerResponse.getResponse()),
callback);
+ return true;
Review Comment:
I wonder if parts of this logic can also live in the super class
`DruidAvaticaHandler`. Not a blocker for this PR though.
##########
server/src/test/java/org/apache/druid/server/AsyncManagementForwardingServletTest.java:
##########
@@ -534,12 +532,11 @@ public String getCurrentLeader()
AuthenticationUtils.addAuthenticationFilterChain(root,
ImmutableList.of(new AllowAllAuthenticator()));
JettyServerInitUtils.addExtensionFilters(root, injector);
- final HandlerList handlerList = new HandlerList();
- handlerList.setHandlers(
- new Handler[]{JettyServerInitUtils.wrapWithDefaultGzipHandler(
+ final Handler.Sequence handlerList = new Handler.Sequence(
+ JettyServerInitUtils.wrapWithDefaultGzipHandler(
root,
ServerConfig.DEFAULT_GZIP_INFLATE_BUFFER_SIZE,
- Deflater.DEFAULT_COMPRESSION)}
+ Deflater.DEFAULT_COMPRESSION)
Review Comment:
```suggestion
Deflater.DEFAULT_COMPRESSION
)
```
##########
server/src/test/java/org/apache/druid/server/mocks/MockHttpServletResponse.java:
##########
@@ -145,9 +149,13 @@ public void addDateHeader(String name, long date)
throw new UnsupportedOperationException();
}
+ /**
+ * HttpServletResponse 4.0.1 spec dictates that setHeader overwrites
existing values.
Review Comment:
Nit: Since this must already be specified in the super javadoc, we can just
make this a regular (non-javadoc) comment inside the method body.
##########
server/src/test/java/org/apache/druid/server/initialization/jetty/StandardResponseHeaderFilterHolderTest.java:
##########
@@ -130,6 +139,40 @@ public void test_get_invalidContentSecurityPolicy()
);
}
+ @Test
+ public void test_deduplicateHeadersInProxyServlet_duplicatesExist()
+ {
+
EasyMock.expect(proxyResponse.containsHeader("Cache-Control")).andReturn(true).once();
+ proxyResponse.setHeader("Cache-Control", null);
+ EasyMock.expectLastCall().once();
+
EasyMock.expect(proxyResponse.containsHeader("Strict-Transport-Security")).andReturn(false).once();
+
+ EasyMock.expect(clientResponse.getHeaders()).andReturn(HttpFields.from(new
HttpField("Cache-Control", "true"), new HttpField("Strict-Transport-Security",
"true"))).times(3);
+
+
+ replayAllMocks();
+
+
StandardResponseHeaderFilterHolder.deduplicateHeadersInProxyServlet(proxyResponse,
clientResponse);
+ }
+
+ @Test
+ public void test_duplicateHeadersInProxyServlet_noDuplicates()
Review Comment:
```suggestion
public void test_deduplicateHeadersInProxyServlet_withNoDuplicates()
```
##########
sql/src/test/java/org/apache/druid/sql/avatica/DruidAvaticaHandlerTest.java:
##########
@@ -255,7 +255,7 @@ protected String getJdbcUrlTail()
}
// Default implementation is for JSON to allow debugging of tests.
- protected AbstractAvaticaHandler getAvaticaHandler(final DruidMeta druidMeta)
+ protected Handler.Abstract getAvaticaHandler(final DruidMeta druidMeta)
Review Comment:
```suggestion
protected DruidAvaticaHandler getAvaticaHandler(final DruidMeta druidMeta)
```
--
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]
