Handle unique query id on server #9648
Conversation
Codecov Report
@@ Coverage Diff @@
## master #9648 +/- ##
=============================================
- Coverage 62.83% 28.05% -34.79%
+ Complexity 5162 53 -5109
=============================================
Files 1935 1936 +1
Lines 103815 103816 +1
Branches 15758 15754 -4
=============================================
- Hits 65236 29123 -36113
- Misses 33708 71817 +38109
+ Partials 4871 2876 -1995
Flags with carried forward coverage won't be shown. Click here to find out more.
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
| @@ -201,23 +191,12 @@ public boolean cancelQuery(long requestId, int timeoutMs, Executor executor, Htt | |||
| if (queryServers == null) { | |||
| return false; | |||
| } | |||
| // TODO: Use different global query id for OFFLINE and REALTIME table after releasing 0.12.0. See QueryIdUtils for | |||
There was a problem hiding this comment.
Are we going to log different global query ids(or traceId as mentioned in this PR) for the same query in the broker log as well? That can help triage the queries in different components in the future.
There was a problem hiding this comment.
With this change, we will always log the same request id on both broker and servers. Query id and trace id are only available within the server, so you won't find them on the broker.
There was a problem hiding this comment.
thanks for reverting the breaking stuff here. I'm going to think about this some more and see if there's a more generic solution here. Not a big fan of <broker>_<server>_<O|R> as some magical string, but doing some interface RequestID seems like overkill at the moment
| @@ -65,10 +67,20 @@ public class QueryResource { | |||
| @ApiResponse(code = 404, message = "Query not found running on the server") | |||
| }) | |||
| public String cancelQuery( | |||
| @ApiParam(value = "QueryId as in the format of <brokerId>_<requestId>", required = true) @PathParam("queryId") | |||
| String queryId) { | |||
| @ApiParam(value = "QueryId as in the format of <brokerId>_<requestId> or <brokerId>_<requestId>_(O|R)", | |||
There was a problem hiding this comment.
nit: it might make more sense for consistency with other APIs to make the offline/realtime part another optional API param. The default behavior has always been to cancel both, so I don't think there's backwards compatibility to worry about
There was a problem hiding this comment.
This API is on the server side, which is meant to be called by the broker. We need the type suffix to separate the OFFLINE and REALTIME query on the server side. Since they are tracked as 2 separate queries, it is better to give finer control.
On the broker side, the query cancellation will take the request id and always cancel both queries. When the server is upgraded, broker can cancel OFFLINE queries and REALTIME queries individually because they might fanout to different servers.
This PR reverts the special broker negative request id handling for hybrid table introduced in #9443, and move the handling to the server side to avoid all the logging change related to the request id.
On the server side, the unique query id is generated as
<brokerId>_<requestId>_(O|R)for OFFLINE and REALTIME request correspondingly. For backward compatibility, when trying to cancel a query without the type suffix, server will look up both OFFLINE and REALTIME query and cancel both of them.One behavior change is that when looking up the running queries on the server side, the returned query ids will have type suffix. This should be desired to reflect the actual queries running.