github-actions[bot] commented on code in PR #67001:
URL: https://github.com/apache/doris/pull/67001#discussion_r3822289434
##########
fe/fe-core/src/main/java/org/apache/doris/nereids/rules/RuleType.java:
##########
@@ -420,8 +420,6 @@ public enum RuleType {
OPERATIVE_COLUMN_DERIVE(RuleTypeClass.REWRITE),
- // topn opts
- DEFER_MATERIALIZE_TOP_N_RESULT(RuleTypeClass.REWRITE),
Review Comment:
[P1] Keep retired rule names parseable across FE upgrades
`disable_nereids_rules` is stored as a free-form session/global string, and
`getDisableNereidsRules()` later resolves every token with
`RuleType.valueOf()`. Before this patch, `DEFER_MATERIALIZE_TOP_N_RESULT` was a
valid value—the changed `EliminateOrderByKeyTest` used it. If an operator has
persisted `SET GLOBAL disable_nereids_rules='DEFER_MATERIALIZE_TOP_N_RESULT'`,
the value is restored and cloned into new sessions after upgrade, where
ordinary Nereids planning now throws `IllegalArgumentException`. Please retain
these retired names as inert compatibility sentinels, or explicitly
ignore/migrate them when constructing the disabled-rule bitset, and add
stale-disable-list upgrade coverage.
##########
be/src/service/internal_service.cpp:
##########
@@ -2240,22 +2240,13 @@ void
PInternalServiceImpl::response_slave_tablet_pull_rowset(
void PInternalService::multiget_data(google::protobuf::RpcController*
controller,
const PMultiGetRequest* request,
PMultiGetResponse* response,
google::protobuf::Closure* done) {
- bool ret = _heavy_work_pool.try_offer([request, response, done]() {
- signal::SignalTaskIdKeeper keeper(request->query_id());
- // multi get data by rowid
- MonotonicStopWatch watch;
- watch.start();
- brpc::ClosureGuard closure_guard(done);
- response->mutable_status()->set_status_code(0);
-
SCOPED_ATTACH_TASK(ExecEnv::GetInstance()->rowid_storage_reader_tracker());
- Status st = RowIdStorageReader::read_by_rowids(*request, response);
- st.to_protobuf(response->mutable_status());
- LOG(INFO) << "multiget_data finished, cost(us):" <<
watch.elapsed_time() / 1000;
- });
- if (!ret) {
- offer_failed(response, done, _heavy_work_pool);
- return;
- }
+ brpc::ClosureGuard closure_guard(done);
+#pragma GCC diagnostic push
+#pragma GCC diagnostic ignored "-Wdeprecated-declarations"
+ // The deprecated response field is retained only to reject legacy callers
explicitly.
+ Status::NotSupported("multiget_data is deprecated; use multiget_data_v2")
Review Comment:
[P1] Preserve the legacy BE path through the 4.1 patch-upgrade window
This targets branch-4.1 after the released 4.1.3 tag, while Doris's [4.x
upgrade
contract](https://doris.apache.org/docs/4.x/admin-manual/cluster-management/upgrade/)
supports same-minor patch rolling upgrades and explicitly upgrades BEs before
FEs. A 4.1.3 FE can still select this path when
`topn_lazy_materialization_threshold <= 0` and the default
`enable_two_phase_read_opt=true`; its fetcher fans `multiget_data` out to every
query-available BE. As soon as one BE is upgraded, this `NotSupported` status
aborts the query, and this patch also removes the phase-one row-ID and
result-sink consumers if the old fragment itself lands on a new BE. Keeping
field numbers does not preserve execution compatibility. Please retain the
complete legacy BE consumer until the released old-FE window closes and add
old-FE/new-BE coverage.
--
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]
