Copilot commented on code in PR #18424:
URL: https://github.com/apache/druid/pull/18424#discussion_r2302979721
##########
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
+ proxyResponse.setHeader(headerName, null);
Review Comment:
Setting a header to `null` may not reliably remove it across all servlet
implementations. Consider using a more explicit removal method or documenting
this behavior's dependency on the specific servlet container.
```suggestion
proxyResponse.removeHeader(headerName);
```
##########
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:
The string-based filtering `!jarFile.getName().contains(\"jetty\")` is
fragile and could accidentally exclude non-Jetty JARs with 'jetty' in their
name. Consider using a more specific pattern match or maintaining an explicit
list of JARs to exclude.
##########
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
{
+ private static final Logger LOG = new Logger(DruidAvaticaJsonHandler.class);
+
public static final String AVATICA_PATH_NO_TRAILING_SLASH =
"/druid/v2/sql/avatica";
public static final String AVATICA_PATH = AVATICA_PATH_NO_TRAILING_SLASH +
"/";
+
+ private final Service service;
+ private final MetricsSystem metrics;
+ private final JsonHandler jsonHandler;
+ private final Timer requestTimer;
+ private final ThreadLocal<UnsynchronizedBuffer> threadLocalBuffer;
+
@Inject
public DruidAvaticaJsonHandler(
final DruidMeta druidMeta,
@Self final DruidNode druidNode,
final AvaticaMonitor avaticaMonitor
)
{
- super(new LocalService(druidMeta), avaticaMonitor);
+ super();
+ this.service = new LocalService(druidMeta);
+ this.metrics = Objects.requireNonNull(avaticaMonitor);
+ this.jsonHandler = new JsonHandler(service, this.metrics);
+
+ // Metrics
+ this.requestTimer = this.metrics.getTimer(
+ MetricsHelper.concat(AvaticaJsonHandler.class,
MetricsAwareAvaticaHandler.REQUEST_TIMER_NAME));
+
+ this.threadLocalBuffer =
ThreadLocal.withInitial(UnsynchronizedBuffer::new);
+
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
{
- if
(AVATICA_PATH_NO_TRAILING_SLASH.equals(StringUtils.maybeRemoveTrailingSlash(request.getRequestURI())))
{
- super.handle(target, baseRequest, request, response);
+ String requestURI = request.getHttpURI().getPath();
+ if
(AVATICA_PATH_NO_TRAILING_SLASH.equals(StringUtils.maybeRemoveTrailingSlash(requestURI)))
{
+ try (Timer.Context ctx = requestTimer.start()) {
+ response.getHeaders().put("Content-Type",
"application/json;charset=utf-8");
+
+ if ("POST".equals(request.getMethod())) {
+ String rawRequest = request.getHeaders().get("request");
+ if (rawRequest == null) {
+ // Avoid a new buffer creation for every HTTP request
+ final UnsynchronizedBuffer buffer = threadLocalBuffer.get();
+ try (InputStream inputStream =
Content.Source.asInputStream(request)) {
+ byte[] bytes = AvaticaUtils.readFullyToBytes(inputStream,
buffer);
+ String encoding = request.getHeaders().get("Content-Encoding");
+ if (encoding == null) {
+ encoding = "UTF-8";
+ }
+ rawRequest = AvaticaUtils.newString(bytes, encoding);
+ }
+ finally {
+ // Reset the offset into the buffer after we're done
+ buffer.reset();
+ }
+ }
+ final String jsonRequest = rawRequest;
+ LOG.trace("request: %s", jsonRequest);
+
+ org.apache.calcite.avatica.remote.Handler.HandlerResponse<String>
jsonResponse;
+ try {
+ jsonResponse = jsonHandler.apply(jsonRequest);
+ }
+ catch (Exception e) {
+ LOG.debug(e, "Error invoking request");
+ jsonResponse = jsonHandler.convertToErrorResponse(e);
+ }
+
+ LOG.trace("response: %s", jsonResponse);
+ response.setStatus(jsonResponse.getStatusCode());
+ response.write(true,
ByteBuffer.wrap(jsonResponse.getResponse().getBytes(StandardCharsets.UTF_8)),
callback);
+ }
+
+ callback.succeeded();
+ return true;
Review Comment:
The callback.succeeded() is called unconditionally, even for non-POST
requests that don't write any response. This should only be called after
successfully writing a response, or the logic should be restructured to handle
different response scenarios correctly.
```suggestion
callback.succeeded();
return true;
} else {
// For non-POST requests, return 405 Method Not Allowed
response.setStatus(405);
response.getHeaders().put("Content-Type",
"application/json;charset=utf-8");
String errorJson = "{\"error\": \"Method Not Allowed\"}";
response.write(true,
ByteBuffer.wrap(errorJson.getBytes(StandardCharsets.UTF_8)), callback);
callback.succeeded();
return true;
}
```
--
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]
