gortiz commented on code in PR #15208:
URL: https://github.com/apache/pinot/pull/15208#discussion_r1986800082
##########
pinot-integration-tests/src/test/java/org/apache/pinot/integration/tests/CancelQueryIntegrationTests.java:
##########
@@ -176,24 +176,32 @@ public void testInstancesStarted() {
@Test(dataProvider = "useBothQueryEngines")
public void testCancelByClientQueryId(boolean useMultiStageQueryEngine)
- throws Exception {
+ throws Exception {
setUseMultiStageQueryEngine(useMultiStageQueryEngine);
String clientRequestId = UUID.randomUUID().toString();
// tricky query: use sleep with some column data to avoid Calcite from
optimizing it on compile time
String sqlQuery =
"SET clientQueryId='" + clientRequestId + "'; "
+ "SELECT sleep(ActualElapsedTime+60000) FROM mytable WHERE
ActualElapsedTime > 0 limit 1";
- new Timer().schedule(new java.util.TimerTask() {
- @Override
- public void run() {
+ Executors.newSingleThreadExecutor().submit(() -> {
Review Comment:
I know this is a test, but we need to be formal. These threads need to be
collected after the test finishes
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseSingleStageBrokerRequestHandler.java:
##########
@@ -755,19 +755,14 @@ protected BrokerResponse doHandleRequest(long requestId,
String query, SqlNodeAn
if (isQueryCancellationEnabled()) {
// Start to track the running query for cancellation just before sending
it out to servers to avoid any
// potential failures that could happen before sending it out, like
failures to calculate the routing table etc.
- // TODO: Even tracking the query as late as here, a potential race
condition between calling cancel API and
- // query being sent out to servers can still happen. If cancel
request arrives earlier than query being
- // sent out to servers, the servers miss the cancel request and
continue to run the queries. The users
- // can always list the running queries and cancel query again
until it ends. Just that such race
- // condition makes cancel API less reliable. This should be rare
as it assumes sending queries out to
- // servers takes time, but will address later if needed.
String clientRequestId = extractClientRequestId(sqlNodeAndOptions);
- onQueryStart(
- requestId, clientRequestId, query, new QueryServers(query,
offlineRoutingTable, realtimeRoutingTable));
+ final Map<ServerInstance, ServerRouteInfo> finalOfflineRoutingTable =
offlineRoutingTable;
+ final Map<ServerInstance, ServerRouteInfo> finalRealtimeRoutingTable =
realtimeRoutingTable;
Review Comment:
nit: we don't usually add final to variables. There is no advantage on doing
so and makes lines longer. Sometimes it may be useful to make it clear that it
won't be changed, but that should be the general case so unless you think it is
especially interesting to show that (ie a final variable declared on a method
where tons of other variables are modified) I think it is better to not add the
final qualifier
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseSingleStageBrokerRequestHandler.java:
##########
@@ -2020,7 +2015,7 @@ protected abstract BrokerResponseNative
processBrokerRequest(long requestId, Bro
@Nullable Map<ServerInstance, ServerRouteInfo> offlineRoutingTable,
@Nullable BrokerRequest realtimeBrokerRequest,
@Nullable Map<ServerInstance, ServerRouteInfo> realtimeRoutingTable,
long timeoutMs,
- ServerStats serverStats, RequestContext requestContext)
+ ServerStats serverStats, RequestContext requestContext, @Nullable
Runnable runningHandler)
Review Comment:
We need to document this new parameter. Otherwise, we will forget its actual
meaning in a week. The name is also not very deterministic. `runningHandler`
may mean anything. Reading the code,, it looks to me that it is a post-send
hook.
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseBrokerRequestHandler.java:
##########
@@ -294,7 +294,7 @@ protected String extractClientRequestId(SqlNodeAndOptions
sqlNodeAndOptions) {
?
sqlNodeAndOptions.getOptions().get(Broker.Request.QueryOptionKey.CLIENT_QUERY_ID)
: null;
}
- protected void onQueryStart(long requestId, String clientRequestId, String
query, Object... extras) {
+ protected void onQueryRunning(long requestId, String clientRequestId, String
query, Object... extras) {
Review Comment:
The fact that we are changing this method's semantics in this PR indicates
that the lifecycle method was not well defined (when it is called and what it
can do). Can we add a javadoc defining that?
##########
pinot-broker/src/main/java/org/apache/pinot/broker/requesthandler/BaseSingleStageBrokerRequestHandler.java:
##########
@@ -2020,7 +2015,7 @@ protected abstract BrokerResponseNative
processBrokerRequest(long requestId, Bro
@Nullable Map<ServerInstance, ServerRouteInfo> offlineRoutingTable,
@Nullable BrokerRequest realtimeBrokerRequest,
@Nullable Map<ServerInstance, ServerRouteInfo> realtimeRoutingTable,
long timeoutMs,
- ServerStats serverStats, RequestContext requestContext)
+ ServerStats serverStats, RequestContext requestContext, @Nullable
Runnable runningHandler)
Review Comment:
BTW, these callback make it more difficult to follow the code flow. They are
fine if they are the only option, but in this case... couldn't we create an
abstract `sendRequest` method, then always call `onQueryRunning` (maybe
renaming it to onQuerySubmitted) and then call a reduce method? That would make
the API easier to understand
##########
pinot-query-runtime/src/main/java/org/apache/pinot/query/service/dispatch/QueryDispatcher.java:
##########
@@ -142,10 +142,19 @@ public void start() {
public QueryResult submitAndReduce(RequestContext context,
DispatchableSubPlan dispatchableSubPlan, long timeoutMs,
Map<String, String> queryOptions)
throws Exception {
+ return submitAndReduce(context, dispatchableSubPlan, timeoutMs,
queryOptions, null);
+ }
+
+ public QueryResult submitAndReduce(RequestContext context,
DispatchableSubPlan dispatchableSubPlan, long timeoutMs,
+ Map<String, String> queryOptions, @Nullable Runnable beforeReduce)
+ throws Exception {
Review Comment:
Same here. Could we split both methods and call the runnable between them?
That would make the API easier to understand.
--
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]
