github-actions[bot] commented on code in PR #65571:
URL: https://github.com/apache/doris/pull/65571#discussion_r3578167946
##########
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 early return skips the validation that the old stream-load path
performed through `CloudEnv.changeCloudCluster(request.getCloudCluster(),
ctx)`. For ordinary stream load, the later `OlapTableSink.createLocation()`
path still calls `CloudSystemInfoService.getCurrentClusterId()`, which rechecks
compute-group privilege/status. The group-commit path is different: when
`group_commit` is set, `NereidsLoadPlanInfoCollector` builds a
`GroupCommitBlockSink`, and that sink intentionally skips `createLocation()` by
returning placeholder locations. The scan side only initializes
`FederationBackendPolicy` from the session cluster, so it does not recover the
skipped `checkCloudClusterPriv`/auto-start/status validation. A user can
therefore route a group-commit stream load to a BE in a compute group they are
not allowed to use and have FE plan it under that backend's group. Please
validate the backend-derived compute group before returning here, or keep the
backend-id shortcut limited to pa
ths that still run the same `changeCloudCluster` checks.
##########
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 compatibility fallback that older FEs understand for
normal password-authenticated stream load. In this PR the FE side is changed
from `if ((request.isSetToken() || request.isSetAuthCode()) &&
request.isSetBackendId())` to accepting any `backend_id`, which is exactly what
makes the new BE-side field work. But during a rolling upgrade a new BE can
still call an older FE: that FE ignores `backend_id` unless the request used
token/auth-code, and after this deletion it no longer receives `cloud_cluster`
from the original `compute_group`/`cloud_cluster` header. A load redirected by
`compute_group` can then be planned in the user's default/policy cluster
instead of the selected BE's compute group. Please keep setting the legacy
`cloud_cluster` field as a fallback while also setting `backend_id`, so old FEs
keep the previous behavior and new FEs can still derive the cluster from the
receiving backend.
--
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]
