imply-cheddar commented on code in PR #12867:
URL: https://github.com/apache/druid/pull/12867#discussion_r955677757
##########
processing/src/test/java/org/apache/druid/query/DefaultQueryMetricsTest.java:
##########
@@ -67,6 +67,7 @@ public void testDefaultQueryMetricsQuery()
.build();
queryMetrics.query(query);
queryMetrics.reportQueryTime(0).emit(serviceEmitter);
+ queryMetrics.sqlQueryId("dummy"); // done just to pacify the code coverage
tool
Review Comment:
What's the point of this comment? 6 months from now, when someone reads
this code and sees that comment, how did the comment enrich their life? Fwiw,
I'm not asking this sarcastically, I'm askign because I want whatever your
answer is to be bundled into the comment :).
That or maybe the test can validate that something is done with the
sqlQueryId and then it can actually be testing it or something?
##########
services/src/test/java/org/apache/druid/server/AsyncQueryForwardingServletTest.java:
##########
@@ -525,30 +555,62 @@ public int read()
return b;
}
};
+ final HttpServletRequest requestMock =
EasyMock.createMock(HttpServletRequest.class);
+ final boolean isAnySql = isJDBCSql || isNativeSql;
EasyMock.expect(requestMock.getContentType()).andReturn("application/json").times(2);
requestMock.setAttribute("org.apache.druid.proxy.objectMapper",
jsonMapper);
EasyMock.expectLastCall();
- EasyMock.expect(requestMock.getRequestURI()).andReturn(isSql ?
"/druid/v2/sql" : "/druid/v2/");
+ EasyMock.expect(requestMock.getRequestURI())
+ .andReturn(isNativeSql ? "/druid/v2/sql" : (isJDBCSql ?
"/druid/v2/sql/avatica" : "/druid/v2/"));
EasyMock.expect(requestMock.getMethod()).andReturn("POST");
-
EasyMock.expect(requestMock.getInputStream()).andReturn(servletInputStream);
requestMock.setAttribute(
- isSql ? "org.apache.druid.proxy.sqlQuery" :
"org.apache.druid.proxy.query",
- query
+ "org.apache.druid.proxy." + (isNativeSql ? "sqlQuery" : (isJDBCSql ?
"avaticaQuery" : "query")),
+ isJDBCSql ? jsonMapper.writeValueAsBytes(query) : query
);
+ EasyMock.expectLastCall();
+
EasyMock.expect(requestMock.getInputStream()).andReturn(servletInputStream);
+
+ // metrics related mocking
+
EasyMock.expect(requestMock.getAttribute("org.apache.druid.proxy.avaticaQuery"))
+ .andReturn(isJDBCSql ? query : null);
+ EasyMock.expect(requestMock.getAttribute("org.apache.druid.proxy.query"))
+ .andReturn(isJDBCSql ? null : (isNativeSql ? null : query));
+
EasyMock.expect(requestMock.getAttribute("org.apache.druid.proxy.sqlQuery"))
+ .andReturn(isJDBCSql ? null : (isNativeSql ? query : null));
+
EasyMock.expect(requestMock.getRemoteAddr()).andReturn("0.0.0.0:0").times(isJDBCSql
? 1 : 2);
requestMock.setAttribute("org.apache.druid.proxy.to.host", "1.2.3.4:9999");
+ EasyMock.expectLastCall();
requestMock.setAttribute("org.apache.druid.proxy.to.host.scheme", "http");
EasyMock.expectLastCall();
EasyMock.replay(requestMock);
final AtomicLong didService = new AtomicLong();
+ final Request proxyRequestMock = Mockito.spy(Request.class);
+ final Result result = new Result(
+ proxyRequestMock,
+ new HttpResponse(proxyRequestMock, ImmutableList.of())
+ {
+ @Override
+ public HttpFields getHeaders()
+ {
+ HttpFields httpFields = new HttpFields();
+ httpFields.add(new
HttpField(QueryResource.QUERY_ID_RESPONSE_HEADER, "dummy"));
Review Comment:
I tend to frown on re-using constants like this in a test. The test is
validating the consistency of the API. If you use a constant like this for
this part, then someone could come along and change the header that the queryId
is returned on, the tests would pass because they are also being changed
because they are using the same object, but the production deployment could
fail as you've broken the API: anything that depended on the older header name
will be broken.
It's better for tests to actually be brittle in these cases: hard-code the
header name so that if anything accidentally changes it in the future, it will
be caught by the tests.
##########
services/src/main/java/org/apache/druid/server/AsyncQueryForwardingServlet.java:
##########
@@ -292,6 +301,18 @@ protected void service(HttpServletRequest request,
HttpServletResponse response)
doService(request, response);
}
+ /**
+ * Rebuilds the {@link SqlQuery} object with a sqlQueryId context paramenter
if not present
+ * @param sqlQuery the original SqlQuery
+ * @return an updated sqlQuery object with sqlQueryId context parameter
+ */
+ private SqlQuery buildSqlQueryWithId(SqlQuery sqlQuery)
+ {
+ Map<String, Object> context = new HashMap<>(sqlQuery.getContext());
+ context.putIfAbsent(BaseQuery.SQL_QUERY_ID, UUID.randomUUID().toString());
Review Comment:
Just double checking, but this will end up setting the native queryId as
well if that was null, right? I.e. when I'm comparing my `query/time` metrics
filtering on a single native queryId, I'll also get the `query/time` from the
router, right?
--
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]
