NBS-4665, NBS-4262: checkpoints: small fixes

ydb-platform · Jan 31, 2024 · 15cb022 · 15cb022
1 parent 0464a93
commit 15cb022
Show file tree

Hide file tree

Showing 5 changed files with 33 additions and 31 deletions.
diff --git a/cloud/blockstore/libs/storage/volume/model/checkpoint.cpp b/cloud/blockstore/libs/storage/volume/model/checkpoint.cpp
@@ -297,14 +297,21 @@ void TCheckpointStore::Apply(const TCheckpointRequest& checkpointRequest)
     }
 }
 
-TSet<TString>& TCheckpointStore::GetCheckpoitsWithSavedRequest()
+bool TCheckpointStore::HasSavedRequest(const TString& checkpointId) const
 {
-    return CheckpointsWithSavedRequest;
+    return CheckpointsWithSavedRequest.contains(checkpointId);
+}
+
+void TCheckpointStore::MarkHasSavedRequest(TString checkpointId)
+{
+    Y_DEBUG_ABORT_UNLESS(!CheckpointsWithSavedRequest.contains(checkpointId));
+    CheckpointsWithSavedRequest.insert(checkpointId);
 }
 
-const TSet<TString>& TCheckpointStore::GetCheckpoitsWithSavedRequest() const
+void TCheckpointStore::MarkHasNoSavedRequest(TString checkpointId)
 {
-    return CheckpointsWithSavedRequest;
+    Y_DEBUG_ABORT_UNLESS(CheckpointsWithSavedRequest.contains(checkpointId));
+    CheckpointsWithSavedRequest.erase(checkpointId);
 }
 
 ////////////////////////////////////////////////////////////////////////////////

diff --git a/cloud/blockstore/libs/storage/volume/model/checkpoint.h b/cloud/blockstore/libs/storage/volume/model/checkpoint.h
@@ -151,8 +151,9 @@ class TCheckpointStore
 
     [[nodiscard]] TVector<TCheckpointRequest> GetCheckpointRequests() const;
 
-    TSet<TString>& GetCheckpoitsWithSavedRequest();
-    const TSet<TString>& GetCheckpoitsWithSavedRequest() const;
+    bool HasSavedRequest(const TString& checkpointId) const;
+    void MarkHasSavedRequest(TString checkpointId);
+    void MarkHasNoSavedRequest(TString checkpointId);
 
 private:
     [[nodiscard]] TCheckpointRequest& GetRequest(ui64 requestId);

diff --git a/cloud/blockstore/libs/storage/volume/model/checkpoint_ut.cpp b/cloud/blockstore/libs/storage/volume/model/checkpoint_ut.cpp
@@ -191,13 +191,12 @@ Y_UNIT_TEST_SUITE(TCheckpointStore)
         UNIT_ASSERT_VALUES_EQUAL(true, store.IsCheckpointDeleted("checkpoint-4"));
         UNIT_ASSERT_VALUES_EQUAL(false, store.IsCheckpointDeleted("checkpoint-5"));
 
-        const auto& checkpoitsWithSavedRequest = store.GetCheckpoitsWithSavedRequest();
-        UNIT_ASSERT_VALUES_EQUAL(false, checkpoitsWithSavedRequest.contains("checkpoint-1"));
-        UNIT_ASSERT_VALUES_EQUAL(false, checkpoitsWithSavedRequest.contains("checkpoint-2"));
-        UNIT_ASSERT_VALUES_EQUAL(true, checkpoitsWithSavedRequest.contains("checkpoint-3"));
-        UNIT_ASSERT_VALUES_EQUAL(false, checkpoitsWithSavedRequest.contains("checkpoint-4"));
-        UNIT_ASSERT_VALUES_EQUAL(false, checkpoitsWithSavedRequest.contains("checkpoint-5"));
-        UNIT_ASSERT_VALUES_EQUAL(false, checkpoitsWithSavedRequest.contains("checkpoint-6"));
+        UNIT_ASSERT_VALUES_EQUAL(false, store.HasSavedRequest("checkpoint-1"));
+        UNIT_ASSERT_VALUES_EQUAL(false, store.HasSavedRequest("checkpoint-2"));
+        UNIT_ASSERT_VALUES_EQUAL(true, store.HasSavedRequest("checkpoint-3"));
+        UNIT_ASSERT_VALUES_EQUAL(false, store.HasSavedRequest("checkpoint-4"));
+        UNIT_ASSERT_VALUES_EQUAL(false, store.HasSavedRequest("checkpoint-5"));
+        UNIT_ASSERT_VALUES_EQUAL(false, store.HasSavedRequest("checkpoint-6"));
     }
 
     Y_UNIT_TEST(CreateFail)

diff --git a/cloud/blockstore/libs/storage/volume/volume_actor.h b/cloud/blockstore/libs/storage/volume/volume_actor.h
@@ -643,7 +643,7 @@ class TVolumeActor final
         const TCheckpointRequest& request,
         const TCheckpointRequestInfo& info);
 
-    bool TryReplyToCheckpointRequestWithoutSaving(
+    bool TryReplyToCheckpointRequest(
         const NActors::TActorContext& ctx,
         const TCheckpointRequest& request,
         const TCheckpointRequestInfo& info);

diff --git a/cloud/blockstore/libs/storage/volume/volume_actor_checkpoint.cpp b/cloud/blockstore/libs/storage/volume/volume_actor_checkpoint.cpp
@@ -954,7 +954,7 @@ void TVolumeActor::AddCheckpointRequest(
     const TCheckpointRequest& request,
     const TCheckpointRequestInfo& info)
 {
-    if (TryReplyToCheckpointRequestWithoutSaving(ctx, request, info)) {
+    if (TryReplyToCheckpointRequest(ctx, request, info)) {
         return;
     }
 
@@ -971,8 +971,7 @@ void TVolumeActor::AddCheckpointRequest(
         return;
     }
 
-    if (!State->GetCheckpointStore().GetCheckpoitsWithSavedRequest().contains(
-            request.CheckpointId)) {
+    if (!State->GetCheckpointStore().HasSavedRequest(request.CheckpointId)) {
         SaveCheckpointRequest(ctx, request, info);
         return;
     }
@@ -989,15 +988,15 @@ void TVolumeActor::AddCheckpointRequest(
         info});
 }
 
-bool TVolumeActor::TryReplyToCheckpointRequestWithoutSaving(
+bool TVolumeActor::TryReplyToCheckpointRequest(
     const TActorContext& ctx,
     const TCheckpointRequest& request,
     const TCheckpointRequestInfo& info)
 {
-    std::optional<NProto::TError> error =
+    std::optional<NProto::TError> validationResult =
         ValidateCheckpointRequest(ctx, request);
 
-    if (!error) {
+    if (!validationResult) {
         return false;
     }
 
@@ -1006,15 +1005,15 @@ bool TVolumeActor::TryReplyToCheckpointRequestWithoutSaving(
     switch (request.ReqType) {
         case ECheckpointRequestType::Create:
         case ECheckpointRequestType::CreateWithoutData: {
-            response = std::make_unique<TCreateCheckpointMethod::TResponse>(*error);
+            response = std::make_unique<TCreateCheckpointMethod::TResponse>(*validationResult);
             break;
         }
         case ECheckpointRequestType::Delete: {
-            response = std::make_unique<TDeleteCheckpointMethod::TResponse>(*error);
+            response = std::make_unique<TDeleteCheckpointMethod::TResponse>(*validationResult);
             break;
         }
         case ECheckpointRequestType::DeleteData: {
-            response = std::make_unique<TDeleteCheckpointDataMethod::TResponse>(*error);
+            response = std::make_unique<TDeleteCheckpointDataMethod::TResponse>(*validationResult);
             break;
         }
     }
@@ -1041,9 +1040,8 @@ void TVolumeActor::ProcessPostponedCheckpointRequests(
         const auto& request = State->GetCheckpointStore().GetRequestById(
             ponstponedRequestInfo->RequestId);
 
-        if (!TryReplyToCheckpointRequestWithoutSaving(ctx, request, ponstponedRequestInfo->Info)) {
-            if (!State->GetCheckpointStore().GetCheckpoitsWithSavedRequest().contains(
-                    request.CheckpointId)) {
+        if (!TryReplyToCheckpointRequest(ctx, request, ponstponedRequestInfo->Info)) {
+            if (!State->GetCheckpointStore().HasSavedRequest(request.CheckpointId)) {
                 PostponedCheckpointRequests.RemovePostponedRequest(checkpointId);
                 SaveCheckpointRequest(ctx, request, ponstponedRequestInfo->Info);
             }
@@ -1068,8 +1066,7 @@ void TVolumeActor::SaveCheckpointRequest(
     const TCheckpointRequest& request,
     const TCheckpointRequestInfo& info)
 {
-    State->GetCheckpointStore().GetCheckpoitsWithSavedRequest().insert(
-        request.CheckpointId);
+    State->GetCheckpointStore().MarkHasSavedRequest(request.CheckpointId);
 
     AddTransaction(*info.RequestInfo);
 
@@ -1394,9 +1391,7 @@ void TVolumeActor::ExecuteUpdateCheckpointRequest(
     auto checkpointRequestCopy =
         State->GetCheckpointStore().GetRequestById(args.RequestId);
 
-    auto& checkpointsWithSavedRequest =
-        State->GetCheckpointStore().GetCheckpoitsWithSavedRequest();
-    checkpointsWithSavedRequest.erase(checkpointRequestCopy.CheckpointId);
+    State->GetCheckpointStore().MarkHasNoSavedRequest(checkpointRequestCopy.CheckpointId);
 
     TVolumeDatabase db(tx.DB);
     db.UpdateCheckpointRequest(