github-actions[bot] commented on code in PR #63217:
URL: https://github.com/apache/doris/pull/63217#discussion_r3235900137
##########
cloud/src/meta-service/meta_service_resource.cpp:
##########
@@ -325,6 +325,96 @@ static int find_cascade_instances(TxnKv* txn_kv, const
std::string& root_instanc
return 0;
}
+static int find_storage_vault_position_by_id(const InstanceInfoPB& instance,
+ std::string_view vault_id) {
+ auto id_itr =
+ std::find(instance.resource_ids().begin(),
instance.resource_ids().end(), vault_id);
+ if (id_itr == instance.resource_ids().end()) {
+ return -1;
+ }
+ return static_cast<int>(id_itr - instance.resource_ids().begin());
+}
+
+static int find_storage_vault_id_by_name(const InstanceInfoPB& instance,
+ std::string_view vault_name,
std::string* vault_id) {
+ auto name_itr = std::find_if(
+ instance.storage_vault_names().begin(),
instance.storage_vault_names().end(),
+ [&](const auto& current_name) { return current_name == vault_name;
});
+ if (name_itr == instance.storage_vault_names().end()) {
+ return -1;
+ }
+ int pos = static_cast<int>(name_itr -
instance.storage_vault_names().begin());
+ *vault_id = instance.resource_ids().Get(pos);
+ return 0;
+}
+
+static int alter_instance_obj_store_info_by_id(InstanceInfoPB& instance,
+ std::string_view target_obj_id,
std::string_view ak,
+ std::string_view sk,
std::string_view role_arn,
+ std::string_view external_id,
+ const EncryptionInfoPB&
encryption_info,
+ MetaServiceCode& code,
std::string& msg) {
+ auto& obj_info =
const_cast<std::decay_t<decltype(instance.obj_info())>&>(instance.obj_info());
+ for (auto& it : obj_info) {
+ if (it.id() != target_obj_id) {
+ continue;
+ }
+
+ if (role_arn.empty()) {
+ if (it.ak() == ak && it.sk() == sk) {
+ code = MetaServiceCode::OK;
+ msg = "ak/sk not changed";
+ return 1;
+ }
+ it.clear_role_arn();
+ it.clear_external_id();
+ it.clear_cred_provider_type();
+
+ it.set_ak(std::string(ak));
+ it.set_sk(std::string(sk));
+ it.mutable_encryption_info()->CopyFrom(encryption_info);
+ } else {
+ if (!ak.empty() || !sk.empty()) {
+ code = MetaServiceCode::INVALID_ARGUMENT;
+ msg = "invaild argument, both set ak/sk and role_arn is not
allowed";
+ LOG(INFO) << msg;
+ return -1;
+ }
+
+ if (it.provider() != ObjectStoreInfoPB::S3) {
+ code = MetaServiceCode::INVALID_ARGUMENT;
+ msg = "role_arn is only supported for s3 provider";
+ LOG(INFO) << msg << " provider=" << it.provider();
+ return -1;
+ }
+
+ if (it.role_arn() == role_arn && it.external_id() == external_id) {
+ code = MetaServiceCode::OK;
+ msg = "ak/sk not changed";
+ return 1;
+ }
+ it.clear_ak();
+ it.clear_sk();
+ it.clear_encryption_info();
+
+ it.set_role_arn(std::string(role_arn));
+ it.set_external_id(std::string(external_id));
+ it.set_cred_provider_type(CredProviderTypePB::INSTANCE_PROFILE);
+ }
Review Comment:
This extraction regresses role-based obj-store updates: the old code wrote
`get_cred_provider_type(request->obj())` and included that value in the no-op
comparison, but the helper now always stores `INSTANCE_PROFILE` and treats
matching `role_arn`/`external_id` as unchanged regardless of the requested
provider type. An `ALTER_OBJ_INFO` request that supplies a non-default
`cred_provider_type` will be silently persisted as `INSTANCE_PROFILE`, or
ignored if only the provider type changes. Please pass the requested provider
type into the helper and compare/write that value.
##########
cloud/src/meta-service/meta_service_resource.cpp:
##########
@@ -1494,6 +1644,115 @@ void
MetaServiceImpl::alter_storage_vault(google::protobuf::RpcController* contr
code = cast_as<ErrCategory::COMMIT>(err);
msg = fmt::format("failed to commit kv txn, err={}", err);
LOG(WARNING) << msg;
+ return;
+ }
+
+ async_notify_refresh_instance(txn_kv_, instance_id, true);
+
+ if (!supports_cascade) {
+ return;
Review Comment:
The storage-vault cascade is not retry-safe after the root commit. For
`ALTER_*_VAULT` with `alter_name`, the root vault has already been renamed
before this cascade starts. If any later cascade step fails transiently and the
RPC is retried, the retry uses the original `request->vault().name()` and fails
the root lookup because that old name no longer exists, leaving derived
instances stale. This needs an idempotent design, for example looking
up/continuing by stable vault id and tolerating already-renamed root/children,
or otherwise ensuring failures after the root commit are repaired without
requiring the original request to succeed on retry.
##########
cloud/src/meta-service/meta_service_resource.cpp:
##########
@@ -1849,9 +2162,11 @@ void
MetaServiceImpl::update_ak_sk(google::protobuf::RpcController* controller,
std::unique_ptr<Transaction> cascade_txn;
TxnErrorCode cascade_err = txn_kv_->create_txn(&cascade_txn);
if (cascade_err != TxnErrorCode::TXN_OK) {
- LOG(WARNING) << "failed to create txn for derived instance,
instance_id=" << cascade_id
- << " err=" << cascade_err;
- continue;
+ code = cast_as<ErrCategory::CREATE>(cascade_err);
+ msg = fmt::format("failed to create txn for derived instance,
instance_id={}, err={}",
+ cascade_id, cascade_err);
+ LOG(WARNING) << msg;
+ return;
}
Review Comment:
Returning an error here happens after the root instance has already
committed. Retrying `update_ak_sk` with the same request cannot reliably finish
the cascade because `update_instance_ak_sk` rejects unchanged root credentials
as `INVALID_ARGUMENT` (`ak sk eq original`), so a transient failure on a
derived instance can leave the parent updated and children stale with no
successful retry path. Either make the operation idempotent across root and
derived instances, or do not expose a post-root-commit transient as a retryable
failure without a repair path.
--
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]
