Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

YQ-2803 Exception checking in checkpoint storages #1540

Merged
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 9 additions & 10 deletions ydb/core/fq/libs/checkpoint_storage/ydb_checkpoint_storage.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -803,13 +803,11 @@ TFuture<ICheckpointStorage::TCreateCheckpointResult> TCheckpointStorage::CreateC
return CreateCheckpointWrapper(future, checkpointContext);
});

return future.Apply(
[checkpointContext](const TFuture<NYdb::TStatus>& future) {
if (NYql::TIssues issues = StatusToIssues(future.GetValue())) {
return TCreateCheckpointResult(TString(), std::move(issues));
} else {
return TCreateCheckpointResult(checkpointContext->CheckpointGraphDescriptionContext->GraphDescId, NYql::TIssues());
}
return StatusToIssues(future).Apply(
[checkpointContext] (const TFuture<TIssues>& future) {
NYql::TIssues issues = future.GetValue();
TString descId = !issues ? checkpointContext->CheckpointGraphDescriptionContext->GraphDescId : TString();
return TCreateCheckpointResult(descId, std::move(issues));
});
}

Expand Down Expand Up @@ -1097,9 +1095,10 @@ TFuture<ICheckpointStorage::TGetTotalCheckpointsStateSizeResult> TCheckpointStor
return status;
});
});
return future.Apply(
[result](const TFuture<TStatus>& status) {
return std::make_pair(std::move(result->Size), std::move(status.GetValue().GetIssues()));

return StatusToIssues(future).Apply(
[result] (const TFuture<TIssues>& future) {
return std::make_pair(std::move(result->Size), std::move(future.GetValue()));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

везде вместо GetValue предлагают использовать ExtractValue, чтобы делался мув и не было копий

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

У Apply() сигнатура должна быть ...(const TFuture&>()) (future.h), а ExtractValue не констатная. Поэтому так не получается

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

тогда давай убирать бесполезные std::move. в этой строке их две штуки, как я понимаю

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Убрал лишние std::move

});
}

Expand Down
13 changes: 4 additions & 9 deletions ydb/core/fq/libs/checkpoint_storage/ydb_state_storage.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -286,6 +286,7 @@ TFuture<IStateStorage::TGetStateResult> TStateStorage::GetState(
return MakeFuture<IStateStorage::TGetStateResult>(result);
}


auto context = MakeIntrusive<TContext>(
YdbConnection->TablePathPrefix,
taskIds,
Expand Down Expand Up @@ -362,15 +363,9 @@ TFuture<IStateStorage::TCountStatesResult> TStateStorage::CountStates(
});
});

return future.Apply(
[context] (const TFuture<TStatus>& future) {
TCountStatesResult countResult;
countResult.first = context->Count;
const auto& status = future.GetValue();
if (!status.IsSuccess()) {
countResult.second = status.GetIssues();
}
return countResult;
return StatusToIssues(future).Apply(
[context] (const TFuture<TIssues>& future) {
return TCountStatesResult{context->Count, std::move(future.GetValue())};
});
}
TExecDataQuerySettings TStateStorage::DefaultExecDataQuerySettings() {
Expand Down
12 changes: 12 additions & 0 deletions ydb/core/fq/libs/ydb/ut/ya.make
Original file line number Diff line number Diff line change
@@ -0,0 +1,12 @@
UNITTEST_FOR(ydb/core/fq/libs/ydb)

SRCS(
ydb_ut.cpp
)

PEERDIR(
ydb/core/fq/libs/ydb
)

END()

23 changes: 23 additions & 0 deletions ydb/core/fq/libs/ydb/ut/ydb_ut.cpp
Original file line number Diff line number Diff line change
@@ -0,0 +1,23 @@
#include <ydb/core/fq/libs/ydb/ydb.h>

#include <library/cpp/testing/unittest/registar.h>

namespace NFq {

Y_UNIT_TEST_SUITE(TFqYdbTest) {

Y_UNIT_TEST(ShouldStatusToIssuesProcessExceptions)
{
auto promise = NThreading::NewPromise<NYdb::TStatus>();
auto future = promise.GetFuture();
TString text("Test exception");
promise.SetException(text);
NThreading::TFuture<NYql::TIssues> future2 = NFq::StatusToIssues(future);

NYql::TIssues issues = future2.GetValueSync();
UNIT_ASSERT(issues.Size() == 1);
UNIT_ASSERT(issues.ToString().Contains(text));
}
};
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

; лишняя


} // namespace NFq
4 changes: 4 additions & 0 deletions ydb/core/fq/libs/ydb/ya.make
Original file line number Diff line number Diff line change
Expand Up @@ -24,3 +24,7 @@ GENERATE_ENUM_SERIALIZATION(ydb.h)
YQL_LAST_ABI_VERSION()

END()

RECURSE_FOR_TESTS(
ut
)
8 changes: 7 additions & 1 deletion ydb/core/fq/libs/ydb/ydb.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -210,7 +210,13 @@ NYql::TIssues StatusToIssues(const NYdb::TStatus& status) {
TFuture<TIssues> StatusToIssues(const TFuture<TStatus>& future) {
return future.Apply(
[] (const TFuture<TStatus>& future) {
return StatusToIssues(future.GetValue());
try {
return StatusToIssues(future.GetValue());
} catch (...) {
TIssues issues;
issues.AddIssue("StatusToIssues failed with exception: " + CurrentExceptionMessage());
return issues;
}
});
}

Expand Down
Loading