Skip to content

Ignore parse error in system.distributed_ddl_queue #79369

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 15 commits into from
Jun 1, 2025
Merged

Ignore parse error in system.distributed_ddl_queue #79369

merged 15 commits into from
Jun 1, 2025

Conversation

ilejn
Copy link
Contributor

@ilejn ilejn commented Apr 21, 2025 •
edited by azat
Loading

Changelog category (leave one):

  • Not for changelog (changelog entry is not required)

Fixes: #75011

@ilejn
Copy link
Contributor Author

ilejn commented Apr 21, 2025

Addresses #75011

@azat azat self-assigned this Apr 21, 2025
azat
azat reviewed Apr 21, 2025
View reviewed changes
@ilejn
Copy link
Contributor Author

ilejn commented Apr 28, 2025

Hello @azat ,
further suggestions?

azat
azat reviewed May 25, 2025
View reviewed changes
Copy link
Member

@azat azat left a comment

Choose a reason for hiding this comment

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

Sorry for the delay (was on vacation)
LGTM, but let's also make test more stable

@azat azat added the can be tested Allows running workflows for external contributors label May 26, 2025
@clickhouse-gh ClickHouse-GH
Copy link

clickhouse-gh bot commented May 26, 2025 •
edited
Loading

Workflow [PR], commit [e6180de]

@clickhouse-gh clickhouse-gh bot added the pr-not-for-changelog This PR should not be mentioned in the changelog label May 26, 2025
@azat
Copy link
Member

azat commented May 28, 2025

Integration tests failures is related, @ilejn can you please take a look?

@ilejn
Copy link
Contributor Author

ilejn commented May 28, 2025

Please, advise if I should trigger the build.
I don't think that any test failure is related, but 'server died' looks too bad.

azat
azat reviewed May 29, 2025
View reviewed changes
@ilejn
Copy link
Contributor Author

ilejn commented May 30, 2025

AST fuzzer thing is not related.

@azat azat added this pull request to the merge queue May 30, 2025
@azat azat removed this pull request from the merge queue due to a manual request May 30, 2025
azat
azat reviewed May 30, 2025
View reviewed changes
@ilejn
Copy link
Contributor Author

ilejn commented May 31, 2025

@azat , imagine that CH parses queries in system.query_log and in case of an exception returns error to client.
This is exactly what is happening here.

@azat azat enabled auto-merge June 1, 2025 06:41
@azat azat added this pull request to the merge queue Jun 1, 2025
@azat
Copy link
Member

azat commented Jun 1, 2025

imagine that CH parses queries in system.query_log and in case of an exception returns error to client.
This is exactly what is happening here.

I understand your point, but I still believe that until we have some solid examples, we should not ignore other errors.

I am concerned that someone might rely on the cluster field and would prefer to know whether the failure is temporary (and the query can be retried) or permanent.

Merged via the queue into ClickHouse:master with commit ce31d75 Jun 1, 2025
120 checks passed
@robot-ch-test-poll1 robot-ch-test-poll1 added the pr-synced-to-cloud The PR is synced to the cloud repo label Jun 1, 2025
ilejn pushed a commit to Altinity/ClickHouse that referenced this pull request Jun 5, 2025
@azat @ilejn
Merge pull request ClickHouse#79369 from ilejn/ignore_error_distribut…
7a49bd3 
…ed_ddl_queue

Ignore parse error in system.distributed_ddl_queue

conflict resolved
zvonand added a commit to Altinity/ClickHouse that referenced this pull request Jun 19, 2025
@zvonand
Merge pull request #843 from Altinity/backports/24.8.14/79369
3d79cdb 
24.8.14 Backport of ClickHouse#79369 -- Ignore parse error in system.distributed_ddl_queue
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@azat azat azat approved these changes

Assignees

@azat azat

Labels
can be tested Allows running workflows for external contributors pr-not-for-changelog This PR should not be mentioned in the changelog pr-synced-to-cloud The PR is synced to the cloud repo
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

system.distributed_ddl_queue parses the queries that it finds in ZK
3 participants
@ilejn @azat @robot-ch-test-poll1