Skip to content

Commit

Permalink
Revert "issue-2100: make blockstore SS proxy error handling consisten…
Browse files Browse the repository at this point in the history 
…t with filestore SS proxy (to unlock the possibility of unification of the common part of these two implementations) (#2101)" (#2116)
  • Loading branch information
@SvartMetal
SvartMetal authored Sep 23, 2024
1 parent 08b9327 commit e476bcb
Show file tree
Hide file tree
Showing 3 changed files with 57 additions and 30 deletions.
25 changes: 25 additions & 0 deletions cloud/blockstore/libs/storage/ss_proxy/ss_proxy_actor.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,17 @@ namespace {

////////////////////////////////////////////////////////////////////////////////

const THashSet<ui32> 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<NTabletPipe::IClientCache> CreateTabletPipeClientCache(
const TStorageConfig& config)
{
Expand Down Expand Up @@ -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<NKikimrScheme::EStatus>(STATUS_FROM_CODE(error.GetCode()));
if (RetriableTxProxyErrors.count(status)) {
error.SetCode(E_REJECTED);
}
return error;
}

} // namespace NCloud::NBlockStore::NStorage
15 changes: 12 additions & 3 deletions cloud/blockstore/libs/storage/ss_proxy/ss_proxy_actor_modifyscheme.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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: "
Expand Down Expand Up @@ -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;
}
}
Expand Down
47 changes: 20 additions & 27 deletions cloud/blockstore/libs/storage/ss_proxy/ss_proxy_ut.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,6 @@
#include <cloud/blockstore/libs/storage/testlib/test_env.h>
#include <cloud/blockstore/libs/storage/testlib/test_runtime.h>

#include <cloud/storage/core/libs/common/error.h>
#include <cloud/storage/core/libs/common/helpers.h>
#include <cloud/storage/core/libs/kikimr/helpers.h>

Expand Down Expand Up @@ -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();
Expand Down Expand Up @@ -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();
Expand All @@ -1236,7 +1235,7 @@ Y_UNIT_TEST_SUITE(TSSProxyTest)
std::make_unique<TEvSchemeShard::TEvDescribeSchemeResult>();
auto& rec = *response->MutableRecord();
rec.SetStatus(NKikimrScheme::StatusNotAvailable);
rec.SetReason("SS is gone");
rec.SetReason("ss is gone");
Send(
runtime,
event->Sender,
Expand All @@ -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,
Expand Down Expand Up @@ -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());
}

Expand All @@ -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());
}

Expand All @@ -1322,8 +1321,8 @@ Y_UNIT_TEST_SUITE(TSSProxyTest)
modifyHandle);

UNIT_ASSERT_VALUES_EQUAL(
EErrorKind::ErrorRetriable,
GetErrorKind(modifyResponse->GetError()));
modifyResponse->GetStatus(),
E_REJECTED);
}
}

Expand Down Expand Up @@ -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;
Expand All @@ -1400,7 +1399,7 @@ Y_UNIT_TEST_SUITE(TSSProxyTest)
}
}

Y_UNIT_TEST(ShouldReturnRetriableErrorIfSchemeShardIsUnavailableForDeprecatedPath)
Y_UNIT_TEST(ShouldReturnERejectedIfSchemeShardIsUnavailableForDeprecatedPath)
{
TTestEnv env;
auto& runtime = env.GetRuntime();
Expand All @@ -1417,7 +1416,7 @@ Y_UNIT_TEST_SUITE(TSSProxyTest)
std::make_unique<TEvSchemeShard::TEvDescribeSchemeResult>();
auto& record = *response->MutableRecord();
record.SetStatus(NKikimrScheme::StatusNotAvailable);
record.SetReason("SS is gone");
record.SetReason("ss is gone");
Send(
runtime,
event->Sender,
Expand All @@ -1444,11 +1443,9 @@ Y_UNIT_TEST_SUITE(TSSProxyTest)
runtime.GrabEdgeEventRethrow<TEvSSProxy::TEvDescribeVolumeResponse>(
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());
}
}
Expand Down Expand Up @@ -1493,9 +1490,7 @@ Y_UNIT_TEST_SUITE(TSSProxyTest)
runtime.GrabEdgeEventRethrow<TEvSSProxy::TEvDescribeVolumeResponse>(
handle);

UNIT_ASSERT_VALUES_EQUAL(
EErrorKind::ErrorRetriable,
GetErrorKind(response->GetError()));
UNIT_ASSERT_VALUES_EQUAL(E_REJECTED, response->GetStatus());
}

Y_UNIT_TEST(ShouldReturnERejectedIfAnyOfPartitionIdsIsZero)
Expand Down Expand Up @@ -1544,9 +1539,7 @@ Y_UNIT_TEST_SUITE(TSSProxyTest)
runtime.GrabEdgeEventRethrow<TEvSSProxy::TEvDescribeVolumeResponse>(
handle);

UNIT_ASSERT_VALUES_EQUAL(
EErrorKind::ErrorRetriable,
GetErrorKind(response->GetError()));
UNIT_ASSERT_VALUES_EQUAL(E_REJECTED, response->GetStatus());
}
}

Expand Down

0 comments on commit e476bcb

Please sign in to comment.