github-actions[bot] commented on code in PR #65571:
URL: https://github.com/apache/doris/pull/65571#discussion_r3578197769
##########
fe/fe-core/src/main/java/org/apache/doris/load/StreamLoadHandler.java:
##########
@@ -137,11 +137,13 @@ public void setCloudCluster() throws UserException {
if (!request.isSetToken() && !request.isSetAuthCode() &&
!Strings.isNullOrEmpty(userName)) {
ctx.setCurrentUserIdentity(resolveCloudLoadUserIdentity(userName));
}
- if ((request.isSetToken() || request.isSetAuthCode()) &&
request.isSetBackendId()) {
+ if (request.isSetBackendId()) {
Review Comment:
This branch should still run the backend-derived compute group through the
same authorization path as the explicit `cloud_cluster` branch before
returning. FE redirect only checks table `LOAD` privilege before selecting a BE
from the `compute_group`/`cloud_cluster` header, and this new branch then sets
the session/request cluster directly from that BE. For ordinary stream load,
the later `OlapTableSink.createLocation()` path reaches
`CloudSystemInfoService.getCurrentClusterId()` and rechecks
`checkCloudClusterPriv()`, but group commit swaps in `GroupCommitBlockSink`,
whose `initLocationParams()` intentionally skips location creation, so a user
with table `LOAD` but no USAGE on that compute group can still get a
group-commit plan on it. Please use `CloudEnv.changeCloudCluster(computeGroup,
ctx)` or an equivalent `checkCloudClusterPriv()`/existence check here before
setting `request.cloud_cluster`, and add a negative group-commit stream-load
case for a user lacking compute-group USA
GE.
##########
be/src/service/http/action/stream_load.cpp:
##########
@@ -786,10 +786,10 @@ Status StreamLoadAction::_process_put(HttpRequest*
http_req,
request.__set_group_commit_mode(ctx->group_commit_mode);
}
- if (!http_req->header(HTTP_COMPUTE_GROUP).empty()) {
- request.__set_cloud_cluster(http_req->header(HTTP_COMPUTE_GROUP));
- } else if (!http_req->header(HTTP_CLOUD_CLUSTER).empty()) {
- request.__set_cloud_cluster(http_req->header(HTTP_CLOUD_CLUSTER));
+ if (_exec_env->cluster_info()->backend_id != 0) {
Review Comment:
This drops the only cluster field that old FEs use for ordinary
password-authenticated stream loads. In a rolling state with this BE talking to
an old master FE, the new request carries `backend_id`, but the old
`StreamLoadHandler.setCloudCluster()` only honored `backend_id` for
token/auth-code requests; normal user/password stream load fell through to
`request.getCloudCluster()`. Since this branch no longer populates
`cloud_cluster`, a load redirected by `compute_group`/`cloud_cluster` can plan
against the user's default/policy cluster during mixed-version upgrade. Please
keep setting the legacy `cloud_cluster` field from
`compute_group`/`cloud_cluster` alongside `backend_id` for the compatibility
window, or document/enforce an FE-first upgrade order and cover that explicitly.
--
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]
