Skip to content

Fixed backward compatibility in fq-proxy result_writer_actor #5111

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
Jun 3, 2024
Merged

Fixed backward compatibility in fq-proxy result_writer_actor #5111

merged 1 commit into from
Jun 3, 2024

Conversation

@GrigoriyPA
Copy link
Collaborator

@GrigoriyPA GrigoriyPA commented Jun 3, 2024 •
edited
Loading

Changelog entry

Fixed backward compatibility in fq-proxy result_writer_actor

Changelog category

  • Not for changelog (changelog entry is not required)

Additional information

@GrigoriyPA GrigoriyPA requested a review from a team as a code owner June 3, 2024 07:15
@GrigoriyPA GrigoriyPA requested review from Hor911 and dorooleg June 3, 2024 07:15
@github-actions
Copy link

github-actions bot commented Jun 3, 2024 •
edited
Loading

⚪ 2024-06-03 07:16:59 UTC Pre-commit check for 3da1a96 has started.
⚪ 2024-06-03 07:19:26 UTC Build linux-x86_64-relwithdebinfo is running...
🟢 2024-06-03 07:21:04 UTC Build successful.
⚪ 2024-06-03 07:21:19 UTC Tests are running...
🔴 2024-06-03 08:55:20 UTC Some tests failed, follow the links below.

Test history

TESTS PASSED ERRORS FAILED SKIPPED MUTED?
36630 31825 0 7 4792 6

@github-actions
Copy link

github-actions bot commented Jun 3, 2024 •
edited
Loading

⚪ 2024-06-03 07:17:30 UTC Pre-commit check for 3da1a96 has started.
⚪ 2024-06-03 07:19:55 UTC Build linux-x86_64-release-clang14 is running...
🟢 2024-06-03 07:22:00 UTC Build successful.

@github-actions
Copy link

github-actions bot commented Jun 3, 2024 •
edited
Loading

⚪ 2024-06-03 07:19:02 UTC Pre-commit check for 3da1a96 has started.
⚪ 2024-06-03 07:21:36 UTC Build linux-x86_64-release-asan is running...
🟢 2024-06-03 07:23:21 UTC Build successful.
⚪ 2024-06-03 07:23:35 UTC Tests are running...
🔴 2024-06-03 09:19:06 UTC Some tests failed, follow the links below.

Test history

TESTS PASSED ERRORS FAILED SKIPPED MUTED?
9212 9140 0 31 30 11

dorooleg
dorooleg reviewed Jun 3, 2024
View reviewed changes
ydb/core/fq/libs/compute/ydb/result_writer_actor.cpp

void Handle(const TEvYdbCompute::TEvFetchScriptResultResponse::TPtr& ev) {
const auto& response = *ev.Get()->Get();
if (response.Status == NYdb::EStatus::BAD_REQUEST && FetchRowsLimit == 0) {
Copy link
Collaborator

@dorooleg dorooleg Jun 3, 2024

Choose a reason for hiding this comment

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

А точно bad requst только в этом случае вылетает? Не можем мы тут откатиться по другой ошибке?

Copy link
Collaborator Author

@GrigoriyPA GrigoriyPA Jun 3, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Можем и кажется это даже может быть хорошо, например если в результатах какие-то проблемы (например слишком длинные строчки, более 20MB) и новый вычитыватель не справился:

ydb/ydb/core/kqp/proxy_service/kqp_script_executions.cpp

Lines 2170 to 2179 in 1b4620e

if (result.RowsCount() == 0) {
if (ResultSet.rows_size() > 0) {
Finish();
} else {
Finish(Ydb::StatusIds::BAD_REQUEST, "Failed to fetch script result due to size limit");
}
return;
}

Copy link
Collaborator

@dorooleg dorooleg Jun 3, 2024

Choose a reason for hiding this comment

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

Но такого же быть не должно. Это баг получается в этом случае. На сенсорах будем видеть такие затупы, а то можем не заметить деградаций?

Copy link
Collaborator Author

@GrigoriyPA GrigoriyPA Jun 3, 2024

Choose a reason for hiding this comment

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

Сенсоров на такие затупы нету, но эта проблема должна пофикситься когда GetResult будет переписан на стримовый запрос

@GrigoriyPA GrigoriyPA requested a review from dorooleg June 3, 2024 07:50
@GrigoriyPA GrigoriyPA merged commit 4ca8675 into ydb-platform:main Jun 3, 2024
@GrigoriyPA GrigoriyPA deleted the backward-compatibility-fix-for-fq-proxy branch June 3, 2024 10:06
This was referenced Jun 6, 2024
Closed
Closed
Merged
Merged
@serbel324 serbel324 mentioned this pull request Jun 7, 2024
Merged
@GrigoriyPA GrigoriyPA mentioned this pull request Jun 10, 2024
Merged
@niksaveliev niksaveliev mentioned this pull request Jun 10, 2024
Merged
This was referenced Jun 11, 2024
Merged
Merged
@GrigoriyPA GrigoriyPA mentioned this pull request Jun 13, 2024
Merged
This was referenced Jun 14, 2024
Merged
Merged
This was referenced Jun 15, 2024
Merged
Merged
@niksaveliev niksaveliev mentioned this pull request Jun 17, 2024
Closed
@GrigoriyPA GrigoriyPA mentioned this pull request Jun 18, 2024
Merged
@StekPerepolnen StekPerepolnen mentioned this pull request Jun 19, 2024
Merged
This was referenced Jun 19, 2024
Merged
Merged
This was referenced Jun 23, 2024
Merged
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Hor911 Hor911 Hor911 approved these changes

@dorooleg dorooleg Awaiting requested review from dorooleg

Assignees

No one assigned

Labels

new-feature not-for-changelog

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@GrigoriyPA @dorooleg @Hor911