kfaraz commented on code in PR #18569:
URL: https://github.com/apache/druid/pull/18569#discussion_r2377866672
##########
server/src/test/java/org/apache/druid/server/metrics/QueryCountStatsMonitorTest.java:
##########
@@ -55,37 +56,57 @@ public void setUp()
queryCountStatsProvider = new QueryCountStatsProvider()
Review Comment:
Should we just use the new impl `BaseQueryCountResource` here?
##########
server/src/test/java/org/apache/druid/server/QueryResourceTest.java:
##########
@@ -769,7 +777,7 @@ public void testTruncatedResponseContextShouldFail() throws
IOException
SIMPLE_TIMESERIES_QUERY.getBytes(StandardCharsets.UTF_8),
queryResource
);
- Assert.assertEquals(1, queryResource.getInterruptedQueryCount());
+ Assert.assertEquals(1, queryResource.counter.getInterruptedQueryCount());
Review Comment:
Let's not access the `counter` via the `QueryResource`
since it doesn't really belong to the resource and is a global service-level
entity.
We should just hold a reference to the `counter` itself.
##########
server/src/test/java/org/apache/druid/server/QueryResourceTest.java:
##########
@@ -1379,8 +1391,8 @@ public void testTooManyQuery() throws
InterruptedException, ExecutionException
for (Future<Boolean> theFuture : back2) {
Assert.assertTrue(theFuture.get());
}
- Assert.assertEquals(2, queryResource.getSuccessfulQueryCount());
- Assert.assertEquals(1, queryResource.getFailedQueryCount());
+ Assert.assertEquals(2, queryResource.counter.getSuccessfulQueryCount());
+ Assert.assertEquals(1, queryResource.counter.getFailedQueryCount());
Review Comment:
Same.
##########
sql/src/main/java/org/apache/druid/sql/http/SqlResourceQueryResultPusherFactory.java:
##########
@@ -52,7 +53,7 @@ public SqlResourceQueryResultPusherFactory(
this.selfNode = selfNode;
}
- public SqlResourceQueryResultPusher factorize(HttpServletRequest req,
HttpStatement stmt, SqlQuery sqlQuery)
+ public SqlResourceQueryResultPusher factorize(final QueryCountStatsProvider
counter, HttpServletRequest req, HttpStatement stmt, SqlQuery sqlQuery)
Review Comment:
Please put args on separate lines
##########
sql/src/main/java/org/apache/druid/sql/http/SqlHttpModule.java:
##########
@@ -35,6 +37,7 @@ public class SqlHttpModule implements Module
public void configure(Binder binder)
{
binder.bind(SqlResource.class).in(LazySingleton.class);
+
binder.bind(QueryCountStatsProvider.class).to(BaseQueryCountResource.class).in(LazySingleton.class);
Review Comment:
This is already bound on `CliBroker` which is the only service which uses
`SqlHttpModule` (AFAICT).
Do we still need this additional binding?
##########
server/src/test/java/org/apache/druid/server/metrics/QueryCountStatsMonitorTest.java:
##########
@@ -126,6 +155,14 @@ public void testMonitor_emitPendingRequests()
new QueryCountStatsMonitor(queryCountStatsProvider, monitorsConfig,
mergeBufferPool);
final StubServiceEmitter emitter = new StubServiceEmitter("service",
"host");
monitor.doMonitor(emitter);
+
+ // Mock metrics emission
Review Comment:
```suggestion
// Simulate metrics emission
```
##########
server/src/test/java/org/apache/druid/server/QueryResultPusherTest.java:
##########
@@ -134,9 +133,33 @@ public ResultsWriter start()
assertTrue("recordFailure(e) should have been invoked!",
recordFailureInvoked.get());
}
- static class NoopQueryMetricCounter implements QueryMetricCounter
+ static class NoopQueryMetricCounter implements QueryCountStatsProvider
Review Comment:
I suppose this might not be needed anymore either.
We may just use the new impl `BaseQueryCountResource`, unless something in
the test asserts that some metric is zero (which we can update to reflect the
true value).
##########
server/src/main/java/org/apache/druid/server/QueryResource.java:
##########
@@ -97,7 +97,7 @@ public class QueryResource implements QueryCountStatsProvider
private final AtomicLong failedQueryCount = new AtomicLong();
private final AtomicLong interruptedQueryCount = new AtomicLong();
private final AtomicLong timedOutQueryCount = new AtomicLong();
- private final QueryResourceQueryMetricCounter counter = new
QueryResourceQueryMetricCounter();
+ final QueryCountStatsProvider counter;
Review Comment:
```suggestion
private final QueryCountStatsProvider counter;
```
##########
services/src/main/java/org/apache/druid/server/AsyncQueryForwardingServlet.java:
##########
@@ -159,7 +160,8 @@ public AsyncQueryForwardingServlet(
GenericQueryMetricsFactory queryMetricsFactory,
AuthenticatorMapper authenticatorMapper,
Properties properties,
- final ServerConfig serverConfig
+ final ServerConfig serverConfig,
+ QueryCountStatsProvider counter
Review Comment:
Nit: for homogeneity with the other fields
```suggestion
final QueryCountStatsProvider counter
```
##########
server/src/test/java/org/apache/druid/server/QueryResourceTest.java:
##########
@@ -1079,7 +1089,7 @@ public <T> QueryRunner<T>
getQueryRunnerForSegments(Query<T> query, Iterable<Seg
Assert.assertEquals("Query did not complete within configured timeout
period. You can " +
"increase query timeout or tune the performance of
query.", ex.getMessage());
Assert.assertEquals(QueryException.QUERY_TIMEOUT_ERROR_CODE,
ex.getErrorCode());
- Assert.assertEquals(1, timeoutQueryResource.getTimedOutQueryCount());
+ Assert.assertEquals(1,
timeoutQueryResource.counter.getTimedOutQueryCount());
Review Comment:
Same comment here regarding accessing the `counter` via the resource class.
##########
server/src/main/java/org/apache/druid/server/metrics/BaseQueryCountResource.java:
##########
@@ -0,0 +1,85 @@
+/*
+ * 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.server.metrics;
+
+import org.apache.druid.guice.LazySingleton;
+
+import java.util.concurrent.atomic.AtomicLong;
+
+/*
+ A thread-safe query count statistics tracker. Meant to be a shared between
the various query resource handlers present
+ on an instance.
+*/
+@LazySingleton
Review Comment:
No need to annotate since we are already binding it appropriately where
applicable.
##########
sql/src/main/java/org/apache/druid/sql/http/SqlResource.java:
##########
@@ -80,14 +81,14 @@ public class SqlResource
public static final String SQL_HEADER_VALUE = "yes";
private static final Logger log = new Logger(SqlResource.class);
- public static final SqlResourceQueryMetricCounter QUERY_METRIC_COUNTER = new
SqlResourceQueryMetricCounter();
private final AuthorizerMapper authorizerMapper;
private final SqlResourceQueryResultPusherFactory resultPusherFactory;
private final SqlLifecycleManager sqlLifecycleManager;
private final SqlEngineRegistry sqlEngineRegistry;
private final DefaultQueryConfig defaultQueryConfig;
private final ServerConfig serverConfig;
+ final QueryCountStatsProvider counter;
Review Comment:
```suggestion
private final QueryCountStatsProvider counter;
```
##########
sql/src/test/java/org/apache/druid/sql/http/SqlResourceTest.java:
##########
@@ -1869,7 +1876,9 @@ public void testTooManyRequestsAfterTotalLaning() throws
Exception
}
}
Assert.assertEquals(2, success);
+ Assert.assertEquals(2, resource.counter.getSuccessfulQueryCount());
Assert.assertEquals(1, limited);
+ Assert.assertEquals(1, resource.counter.getFailedQueryCount());
Review Comment:
We shouldn't access `counter` via the resource class.
##########
server/src/test/java/org/apache/druid/server/metrics/QueryCountStatsMonitorTest.java:
##########
@@ -106,15 +127,23 @@ public void testMonitor()
new QueryCountStatsMonitor(queryCountStatsProvider, monitorsConfig,
mergeBufferPool);
final StubServiceEmitter emitter = new StubServiceEmitter("service",
"host");
monitor.doMonitor(emitter);
+
+ // Mock metrics emission
+ queryCountStatsProvider.incrementSuccess();
+ queryCountStatsProvider.incrementFailed();
+ queryCountStatsProvider.incrementFailed();
+ queryCountStatsProvider.incrementInterrupted();
+ queryCountStatsProvider.incrementTimedOut();
+
emitter.flush();
// Trigger metric emission
monitor.doMonitor(emitter);
Assert.assertEquals(5, emitter.getNumEmittedEvents());
emitter.verifyValue("query/success/count", 1L);
emitter.verifyValue("query/failed/count", 2L);
- emitter.verifyValue("query/interrupted/count", 3L);
- emitter.verifyValue("query/timeout/count", 4L);
- emitter.verifyValue("query/count", 10L);
Review Comment:
I assume these had to be changed due to the weird increments in the test
`QueryCountStatsProvider` defined above?
##########
server/src/test/java/org/apache/druid/server/metrics/QueryCountStatsMonitorTest.java:
##########
@@ -55,37 +56,57 @@ public void setUp()
queryCountStatsProvider = new QueryCountStatsProvider()
{
- private long successEmitCount = 0;
- private long failedEmitCount = 0;
- private long interruptedEmitCount = 0;
- private long timedOutEmitCount = 0;
+ private final AtomicLong successEmitCount = new AtomicLong(0);
+ private final AtomicLong failedEmitCount = new AtomicLong(0);
+ private final AtomicLong interruptedEmitCount = new AtomicLong(0);
+ private final AtomicLong timedOutEmitCount = new AtomicLong(0);
@Override
public long getSuccessfulQueryCount()
{
- successEmitCount += 1;
- return successEmitCount;
+ return successEmitCount.get();
}
@Override
public long getFailedQueryCount()
{
- failedEmitCount += 2;
- return failedEmitCount;
+ return failedEmitCount.get();
}
@Override
public long getInterruptedQueryCount()
{
- interruptedEmitCount += 3;
- return interruptedEmitCount;
+ return interruptedEmitCount.get();
}
@Override
public long getTimedOutQueryCount()
{
- timedOutEmitCount += 4;
Review Comment:
Why was the increment +4? 😅
##########
sql/src/main/java/org/apache/druid/sql/http/SqlResource.java:
##########
@@ -80,14 +81,14 @@ public class SqlResource
public static final String SQL_HEADER_VALUE = "yes";
private static final Logger log = new Logger(SqlResource.class);
- public static final SqlResourceQueryMetricCounter QUERY_METRIC_COUNTER = new
SqlResourceQueryMetricCounter();
Review Comment:
I suppose this was just a dummy instance that didn't even collect the stats?
##########
server/src/main/java/org/apache/druid/server/metrics/QueryCountStatsProvider.java:
##########
@@ -40,4 +40,24 @@ public interface QueryCountStatsProvider
* Returns the number of timed out queries during the emission period.
*/
long getTimedOutQueryCount();
+
+ /**
+ * Increments successful query count.
+ */
+ void incrementSuccess();
Review Comment:
```suggestion
void incrementSuccessful();
```
##########
server/src/main/java/org/apache/druid/server/metrics/BaseQueryCountResource.java:
##########
@@ -0,0 +1,85 @@
+/*
+ * 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.server.metrics;
+
+import org.apache.druid.guice.LazySingleton;
+
+import java.util.concurrent.atomic.AtomicLong;
+
+/*
+ A thread-safe query count statistics tracker. Meant to be a shared between
the various query resource handlers present
+ on an instance.
+*/
+@LazySingleton
+public class BaseQueryCountResource implements QueryCountStatsProvider
Review Comment:
Please rename this to something like `QueryCountStatsAccumulator` or
`*Tracker` or `*ProviderImpl`.
(`Base*` implies that something extends this and `*Resource` is used for
either REST resource classes or closeable resources).
##########
sql/src/main/java/org/apache/druid/sql/http/SqlHttpModule.java:
##########
@@ -35,6 +37,7 @@ public class SqlHttpModule implements Module
public void configure(Binder binder)
{
binder.bind(SqlResource.class).in(LazySingleton.class);
+
binder.bind(QueryCountStatsProvider.class).to(BaseQueryCountResource.class).in(LazySingleton.class);
Review Comment:
We could however make a `Names.named("sql")` annotation binding here and use
the named binding
in `SqlResource` which would allow separate tracking of sql and native query
counts.
--
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]
