Support DDL operations (including IF EXISTS/IF NOT EXISTS) for objects with type secret #838
Conversation
| case EActivityType::Undefined: | ||
| return TYqlConclusionStatus::Fail("undefined action type"); | ||
| } | ||
| Y_DEBUG_ABORT_UNLESS(operationProto); |
There was a problem hiding this comment.
We return Success in release mode, is that what we want? Maybe we should use Y_ENSURE here and abort the request?
There was a problem hiding this comment.
We don't return Success here: we will simply segfault because we use operationProto :)
I think this is an internal logical error, because all options for ActivityType are listed here. Futher more, the compiler would tell us if additional option exists.
There was a problem hiding this comment.
I would prefer Y_ENSURE to get an exception in release instead of a segfault nonetheless
| if (ev->Get()->GetStatus() == Ydb::StatusIds::PRECONDITION_FAILED && ExistingOk) { | ||
| NRequest::TDialogYQLRequest::TResponse resp; | ||
| this->Send(this->SelfId(), new NRequest::TEvRequestResult<NRequest::TDialogYQLRequest>(std::move(resp))); | ||
| this->Requests.clear(); // Remove history request |
There was a problem hiding this comment.
Why do we remove all requests, if one fails?
There was a problem hiding this comment.
It is rather complicated code :) This second request is here: https://github.com/ydb-platform/ydb/blob/main/ydb/services/metadata/manager/modification.h#L93
It is history insertion and we don't need it in case we haven't inserted main thing.
| switch (schemeOperation.GetOperationCase()) { | ||
| case NKqpProto::TKqpSchemeOperation::kUpsertObject: | ||
| internalContext.SetActivityType(EActivityType::Upsert); | ||
| modifyObjectCommand = std::make_shared<TUpsertObjectCommand<T>>(BuildPatchFromProto(schemeOperation.GetUpsertObject()), manager, controller, internalContext); |
There was a problem hiding this comment.
We make a fresh TOperationsController pointer for each ObjectCommand, and we copy it inside. Also we copy it in TInsertObjectsActor constructor. Do we really need those copies, or maybe we could use unique pointer? Can ObjectCommand remain, if corresponding TOperationsController is destroyed?
There was a problem hiding this comment.
I only refactored this code and I don't know why it is shared. But as I see, it can be simplified as you suggested. Thank you, I will try :)
There was a problem hiding this comment.
I tried to use unique_ptr and noticed it: https://github.com/ydb-platform/ydb/blob/main/ydb/services/metadata/initializer/accessor_init.h#L27
I am afraid that we need shared_ptr :(
There was a problem hiding this comment.
I avoided extra copying here using std::move - it is all that I can do :)
|
Note This is an automated comment that will be appended during run. 🔴 linux-x86_64-relwithdebinfo: some tests FAILED for commit 27ecbff.
🔴 linux-x86_64-release-asan: some tests FAILED for commit 27ecbff.
|
UgnineSirdis
force-pushed
the
query-service-secrets-ddl
branch
from
31f42c3 to
2d1bdf2
Compare
…s with type secret
UgnineSirdis
force-pushed
the
query-service-secrets-ddl
branch
from
2d1bdf2 to
27ecbff
Compare
No description provided.