[Enhancement] Support to specify warehouse for query_plan rest query#57930
[Enhancement] Support to specify warehouse for query_plan rest query#57930zhangbutao wants to merge 3 commits into
Conversation
| ctx.setCurrentUserIdentity(currentUser); | ||
| ctx.setCurrentRoleIds(currentUser); | ||
| ctx.setThreadLocalInfo(); | ||
| if (request.getRequest().headers().contains(WAREHOUSE_KEY)) { |
There was a problem hiding this comment.
please add some unit tests about the new parameters, and what's the response looks like if the specified warehouse doesn't exist.
There was a problem hiding this comment.
I added a UT to post a query_plan request, just like sr flink source connector:
https://github.com/StarRocks/starrocks-connector-for-apache-flink/blob/3e17b7b0531f128fb722b4892fdf608931854202/src/main/java/com/starrocks/connector/flink/manager/StarRocksQueryPlanVisitor.java#L95-#L102
Signed-off-by: zhangbutao <zhangbutao@cmss.chinamobile.com>
zhangbutao
force-pushed
the
support_warehouse_for_rest_query
branch
from
58df471 to
0432095
Compare
Signed-off-by: zhangbutao <zhangbutao@cmss.chinamobile.com>
| String respStr = Objects.requireNonNull(response.body()).string(); | ||
| JSONObject jsonObject = new JSONObject(respStr); | ||
| System.out.println(respStr); | ||
| Assert.assertEquals(500, jsonObject.getInt("status")); |
There was a problem hiding this comment.
this is not expected, FE should respond HTTP 400 BAD request.
There was a problem hiding this comment.
The exception comes from here, which all exception wiil be 500
There was a problem hiding this comment.
should do input parameter check before execute the parameter, or do fine grain exception handling. It is the request's parameter invalidation, should not be considered as a HTTP 5XX error. Usually a HTTP 5XX error means the http client is not doing anything wrong, and can do endless retry without modification of the request as wish.
There was a problem hiding this comment.
@kevincai I changed the code to check and set warehouse before execute the query_plan, so that we can get the real error status and exception.
So this PR is for TableQueryPlanAction, which can solve the sr flink source connector warehouse issue.
For other http Action, i am not sure if need the warehouse. But we can also do similar check like this PR if other http Actions have any need.
There was a problem hiding this comment.
It is good enough to fix only in query_plan API in this PR.
zhangbutao
changed the title
Signed-off-by: zhangbutao <zhangbutao@cmss.chinamobile.com>
|
[Java-Extensions Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
[BE Incremental Coverage Report]✅ pass : 0 / 0 (0%) |
|
the issue is fixed by #58387. This PR is not needed any more. |
2 participants
Why I'm doing:
Refert to the doc: https://github.com/StarRocks/starrocks-connector-for-apache-flink/blob/main/docs/content/connector-source.md
At present, we can not specify a warehouse when using flink to query the sr table. I propose to pass the warehouse info through http header.
This PR can be worked after StarRocks/starrocks-connector-for-apache-flink#423
What I'm doing:
Support to specify warehouse for rest query, like flink sr connector.
Fixes #issue
What type of PR is this:
Does this PR entail a change in behavior?
If yes, please specify the type of change:
Checklist:
Bugfix cherry-pick branch check: