github-actions[bot] commented on code in PR #64148:
URL: https://github.com/apache/doris/pull/64148#discussion_r3654225183
##########
fe/fe-core/src/main/java/org/apache/doris/cloud/rpc/MetaServiceProxy.java:
##########
@@ -318,6 +320,40 @@ public <Response> Response executeRequest(String
methodName, Function<MetaServic
}
}
+ @SuppressWarnings("unchecked")
+ // Prefer the exact status code when this FE binary knows the enum value.
The
+ // response keeps a
+ // legacy-compatible fallback code for old clients, so normalize the
returned
+ // message once here
+ // and let existing callers continue to read status.getCode().
+ private static <Response> Response preferExactStatusCode(Response
response) {
+ if (!(response instanceof Message)) {
+ return response;
+ }
+ Message message = (Message) response;
+ Descriptors.FieldDescriptor statusField =
message.getDescriptorForType().findFieldByName("status");
+ if (statusField == null || !message.hasField(statusField)) {
+ return response;
+ }
+ Object statusObject = message.getField(statusField);
+ if (!(statusObject instanceof Cloud.MetaServiceResponseStatus)) {
+ return response;
+ }
+ Cloud.MetaServiceResponseStatus status =
(Cloud.MetaServiceResponseStatus) statusObject;
+
+ if (!status.hasAuxCode()) {
+ return response;
+ }
+ Cloud.MetaServiceCode code =
Cloud.MetaServiceCode.forNumber(status.getAuxCode());
+ if (code == null || code == status.getCode()) {
+ return response;
+ }
+ Cloud.MetaServiceResponseStatus normalizedStatus =
status.toBuilder().setCode(code).build();
Review Comment:
[P1] Keep `createStage` on its established commit-recovery path
`set_response_status` intentionally represents `KV_TXN_MAYBE_COMMITTED` and
exhausted `KV_TXN_STORE_COMMIT_RETRYABLE` as legacy `KV_TXN_COMMIT_ERR`, but
this rewrites them back to the exact values for every new FE response.
`CloudInternalCatalog.createStage` retries only `KV_TXN_CONFLICT` and
`KV_TXN_COMMIT_ERR`; neither exact commit signal is recognized, so it breaks
and throws immediately. In particular, `CREATE STAGE IF NOT EXISTS` can receive
`MAYBE_COMMITTED` after the stage was actually written; the previous retry
resolved the following `ALREADY_EXISTED` as success, while the normalized
response now reports failure even though the requested stage exists. Please
update the downstream consumers (at least `createStage`) to preserve the legacy
category's bounded retry semantics before promoting these exact codes, and add
FE coverage for this path.
##########
cloud/src/meta-service/meta_service_helper.h:
##########
@@ -41,6 +43,40 @@
#include "resource-manager/resource_manager.h"
namespace doris::cloud {
+inline MetaServiceCode get_legacy_code(MetaServiceCode code) {
+ switch (code) {
+ // These retry signals are handled inside MetaService. Map unresolved
failures to the
+ // corresponding operation error so callers receive a stable
read/commit/create category.
+ case MetaServiceCode::KV_TXN_STORE_GET_RETRYABLE:
+ return MetaServiceCode::KV_TXN_GET_ERR;
+ case MetaServiceCode::KV_TXN_STORE_COMMIT_RETRYABLE:
+ return MetaServiceCode::KV_TXN_COMMIT_ERR;
+ case MetaServiceCode::KV_TXN_STORE_CREATE_RETRYABLE:
+ return MetaServiceCode::KV_TXN_CREATE_ERR;
+ // The commit outcome is uncertain after the store reports
MAYBE_COMMITTED. Map it to a
+ // commit error when MetaService cannot resolve the outcome through its
internal retries.
+ case MetaServiceCode::KV_TXN_MAYBE_COMMITTED:
+ return MetaServiceCode::KV_TXN_COMMIT_ERR;
+ // MS_TOO_BUSY is a overload signal. Map it to KV_TXN_CONFLICT so the BE's
existing
+ // conflict-retry path can retry the request.
+ case MetaServiceCode::MS_TOO_BUSY:
+ return MetaServiceCode::KV_TXN_CONFLICT;
+ // Represent an exhausted MetaService conflict-retry path and prevent
another BE-side
+ // conflict-retry loop.
+ case MetaServiceCode::KV_TXN_CONFLICT:
Review Comment:
[P1] Preserve whether a conflict actually exhausted server retries
This maps every raw `KV_TXN_CONFLICT` to legacy
`KV_TXN_CONFLICT_RETRY_EXCEEDED_MAX_TIMES`, while `aux_code` remains
`KV_TXN_CONFLICT`. When `call_impl` really exhausts its internal conflict
retries, new BEs/FE paths prefer the aux value and start their outer conflict
retry loops again, undoing the previous terminal mapping (with current defaults
an always-conflicting request can expand from 5 handler attempts to 55).
Conversely, when `enable_retry_txn_conflict` is false, the server performs one
attempt but legacy clients see `RETRY_EXCEEDED` and no longer retry; the
existing raw-conflict assertions in `BeginTxnTest`, `CleanTxnLabelTest`, and
`UpdateDeleteBitmapWithException` are contradicted as well. Please leave
ordinary conflicts mapped to themselves and restore the context-specific
conversion to `KV_TXN_CONFLICT_RETRY_EXCEEDED_MAX_TIMES` only in `call_impl`'s
actual exhaustion branch, so both channels carry the terminal code only after
exhaustion.
--
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]
