From 78b30edabd26bfa9607549c9e1c7b79c7e9eb6b3 Mon Sep 17 00:00:00 2001 From: Mikhail Montsev Date: Mon, 23 Sep 2024 18:39:59 +0000 Subject: [PATCH] Revert "issue-2100: make blockstore SS proxy error handling consistent with filestore SS proxy (to unlock the possibility of unification of the common part of these two implementations) (#2101)" This reverts commit 227be9d7f957a60be1ab62735de3eed88e64250f. --- .../libs/storage/ss_proxy/ss_proxy_actor.cpp | 25 ++++++++++ .../ss_proxy/ss_proxy_actor_modifyscheme.cpp | 15 ++++-- .../libs/storage/ss_proxy/ss_proxy_ut.cpp | 47 ++++++++----------- 3 files changed, 57 insertions(+), 30 deletions(-) diff --git a/cloud/blockstore/libs/storage/ss_proxy/ss_proxy_actor.cpp b/cloud/blockstore/libs/storage/ss_proxy/ss_proxy_actor.cpp index c497ebbc4e2..e91b222f9ae 100644 --- a/cloud/blockstore/libs/storage/ss_proxy/ss_proxy_actor.cpp +++ b/cloud/blockstore/libs/storage/ss_proxy/ss_proxy_actor.cpp @@ -18,6 +18,17 @@ namespace { //////////////////////////////////////////////////////////////////////////////// +const THashSet RetriableTxProxyErrors { + NKikimr::NTxProxy::TResultStatus::ProxyNotReady, + NKikimr::NTxProxy::TResultStatus::ProxyShardNotAvailable, + NKikimr::NTxProxy::TResultStatus::ProxyShardTryLater, + NKikimr::NTxProxy::TResultStatus::ProxyShardOverloaded, + NKikimr::NTxProxy::TResultStatus::ExecTimeout, + NKikimr::NTxProxy::TResultStatus::ExecResultUnavailable +}; + +//////////////////////////////////////////////////////////////////////////////// + std::unique_ptr CreateTabletPipeClientCache( const TStorageConfig& config) { @@ -171,4 +182,18 @@ NProto::TError GetErrorFromPreconditionFailed(const NProto::TError& error) return result; } +NProto::TError TranslateTxProxyError(NProto::TError error) +{ + if (FACILITY_FROM_CODE(error.GetCode()) != FACILITY_TXPROXY) { + return error; + } + + auto status = + static_cast(STATUS_FROM_CODE(error.GetCode())); + if (RetriableTxProxyErrors.count(status)) { + error.SetCode(E_REJECTED); + } + return error; +} + } // namespace NCloud::NBlockStore::NStorage diff --git a/cloud/blockstore/libs/storage/ss_proxy/ss_proxy_actor_modifyscheme.cpp b/cloud/blockstore/libs/storage/ss_proxy/ss_proxy_actor_modifyscheme.cpp index 1389faf00d4..5ac98266c78 100644 --- a/cloud/blockstore/libs/storage/ss_proxy/ss_proxy_actor_modifyscheme.cpp +++ b/cloud/blockstore/libs/storage/ss_proxy/ss_proxy_actor_modifyscheme.cpp @@ -134,10 +134,18 @@ void TModifySchemeActor::HandleStatus( break; } + ui32 errorCode = MAKE_SCHEMESHARD_ERROR(SchemeShardStatus); + + if (SchemeShardStatus == NKikimrScheme::StatusMultipleModifications || + SchemeShardStatus == NKikimrScheme::StatusNotAvailable) + { + errorCode = E_REJECTED; + } + ReplyAndDie( ctx, MakeError( - MAKE_SCHEMESHARD_ERROR(SchemeShardStatus), + errorCode, (TStringBuilder() << NKikimrSchemeOp::EOperationType_Name(ModifyScheme.GetOperationType()).data() << " failed with reason: " @@ -178,8 +186,9 @@ void TModifySchemeActor::HandleStatus( ReplyAndDie( ctx, - MakeError(MAKE_TXPROXY_ERROR(status), TStringBuilder() - << "TxProxy failed: " << status)); + TranslateTxProxyError( + MakeError(MAKE_TXPROXY_ERROR(status), TStringBuilder() + << "TxProxy failed: " << status))); break; } } diff --git a/cloud/blockstore/libs/storage/ss_proxy/ss_proxy_ut.cpp b/cloud/blockstore/libs/storage/ss_proxy/ss_proxy_ut.cpp index 9c51d70b65e..8f6f565301b 100644 --- a/cloud/blockstore/libs/storage/ss_proxy/ss_proxy_ut.cpp +++ b/cloud/blockstore/libs/storage/ss_proxy/ss_proxy_ut.cpp @@ -10,7 +10,6 @@ #include #include -#include #include #include @@ -1063,7 +1062,7 @@ Y_UNIT_TEST_SUITE(TSSProxyTest) UNIT_ASSERT(count == 1); } - Y_UNIT_TEST(ShouldReturnERejectedIfSchemeShardDetectsPathIdVersionMismatch) + Y_UNIT_TEST(ShouldReturnERejectedIfIfSchemeShardDetectsPathIdVersionMismatch) { TTestEnv env; auto& runtime = env.GetRuntime(); @@ -1222,7 +1221,7 @@ Y_UNIT_TEST_SUITE(TSSProxyTest) UNIT_ASSERT_VALUES_EQUAL(E_ABORTED, modifyResponse->GetError().GetCode()); } - Y_UNIT_TEST(ShouldReturnRetriableErrorWhenSSIsUnavailable) + Y_UNIT_TEST(ShouldReturnERejectedWhenSSisUnavailable) { TTestEnv env; auto& runtime = env.GetRuntime(); @@ -1236,7 +1235,7 @@ Y_UNIT_TEST_SUITE(TSSProxyTest) std::make_unique(); auto& rec = *response->MutableRecord(); rec.SetStatus(NKikimrScheme::StatusNotAvailable); - rec.SetReason("SS is gone"); + rec.SetReason("ss is gone"); Send( runtime, event->Sender, @@ -1250,7 +1249,7 @@ Y_UNIT_TEST_SUITE(TSSProxyTest) TEvTxUserProxy::TEvProposeTransactionStatus::EStatus::ExecError); auto& rec = response->Record; rec.SetSchemeShardStatus(NKikimrScheme::StatusNotAvailable); - rec.SetSchemeShardReason("SS is gone"); + rec.SetSchemeShardReason("ss is gone"); Send( runtime, event->Sender, @@ -1278,10 +1277,10 @@ Y_UNIT_TEST_SUITE(TSSProxyTest) describeHandle); UNIT_ASSERT_VALUES_EQUAL( - EErrorKind::ErrorRetriable, - GetErrorKind(describeResponse->GetError())); + describeResponse->GetStatus(), + E_REJECTED); UNIT_ASSERT_VALUES_EQUAL( - "SS is gone", + "ss is gone", describeResponse->GetError().GetMessage()); } @@ -1298,10 +1297,10 @@ Y_UNIT_TEST_SUITE(TSSProxyTest) describeHandle); UNIT_ASSERT_VALUES_EQUAL( - EErrorKind::ErrorRetriable, - GetErrorKind(describeResponse->GetError())); + describeResponse->GetStatus(), + E_REJECTED); UNIT_ASSERT_VALUES_EQUAL( - "SS is gone", + "ss is gone", describeResponse->GetError().GetMessage()); } @@ -1322,8 +1321,8 @@ Y_UNIT_TEST_SUITE(TSSProxyTest) modifyHandle); UNIT_ASSERT_VALUES_EQUAL( - EErrorKind::ErrorRetriable, - GetErrorKind(modifyResponse->GetError())); + modifyResponse->GetStatus(), + E_REJECTED); } } @@ -1372,8 +1371,8 @@ Y_UNIT_TEST_SUITE(TSSProxyTest) modifyHandle); UNIT_ASSERT_VALUES_EQUAL( - EErrorKind::ErrorRetriable, - GetErrorKind(modifyResponse->GetError())); + modifyResponse->GetStatus(), + E_REJECTED); } txStatus = TEvTxUserProxy::TEvProposeTransactionStatus::EStatus::WrongRequest; @@ -1400,7 +1399,7 @@ Y_UNIT_TEST_SUITE(TSSProxyTest) } } - Y_UNIT_TEST(ShouldReturnRetriableErrorIfSchemeShardIsUnavailableForDeprecatedPath) + Y_UNIT_TEST(ShouldReturnERejectedIfSchemeShardIsUnavailableForDeprecatedPath) { TTestEnv env; auto& runtime = env.GetRuntime(); @@ -1417,7 +1416,7 @@ Y_UNIT_TEST_SUITE(TSSProxyTest) std::make_unique(); auto& record = *response->MutableRecord(); record.SetStatus(NKikimrScheme::StatusNotAvailable); - record.SetReason("SS is gone"); + record.SetReason("ss is gone"); Send( runtime, event->Sender, @@ -1444,11 +1443,9 @@ Y_UNIT_TEST_SUITE(TSSProxyTest) runtime.GrabEdgeEventRethrow( handle); + UNIT_ASSERT_VALUES_EQUAL(E_REJECTED, response->GetStatus()); UNIT_ASSERT_VALUES_EQUAL( - EErrorKind::ErrorRetriable, - GetErrorKind(response->GetError())); - UNIT_ASSERT_VALUES_EQUAL( - "SS is gone", + "ss is gone", response->GetError().GetMessage()); } } @@ -1493,9 +1490,7 @@ Y_UNIT_TEST_SUITE(TSSProxyTest) runtime.GrabEdgeEventRethrow( handle); - UNIT_ASSERT_VALUES_EQUAL( - EErrorKind::ErrorRetriable, - GetErrorKind(response->GetError())); + UNIT_ASSERT_VALUES_EQUAL(E_REJECTED, response->GetStatus()); } Y_UNIT_TEST(ShouldReturnERejectedIfAnyOfPartitionIdsIsZero) @@ -1544,9 +1539,7 @@ Y_UNIT_TEST_SUITE(TSSProxyTest) runtime.GrabEdgeEventRethrow( handle); - UNIT_ASSERT_VALUES_EQUAL( - EErrorKind::ErrorRetriable, - GetErrorKind(response->GetError())); + UNIT_ASSERT_VALUES_EQUAL(E_REJECTED, response->GetStatus()); } }