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

[DO NOT MERGE] Update clickhouse connect to 0.8.14 #6177

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

clee2000
Copy link
Contributor

Did some verification for the easier python scripts like the revert tracker and the test time generator

Ran the dynamo replicator on a bit of workflow_job
Ran the s3 replicator on a bit of merge_bases
Both seemed fine, but we should still monitor just in case

Do not merge until we are awake and able to revert quickly

Copy link

vercel bot commented Jan 16, 2025

The latest updates on your projects. Learn more about Vercel for Git ↗︎

1 Skipped Deployment
Name Status Preview Updated (UTC)
torchci ⬜️ Ignored (Inspect) Jan 16, 2025 0:26am

@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Jan 16, 2025
Copy link
Contributor

@huydhn huydhn left a comment

Choose a reason for hiding this comment

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

LGTM! I'm trying to search for a public announcement from CH to link here to give more context, but I couldn't find any. The context is all the in their email.

@huydhn
Copy link
Contributor

huydhn commented Jan 16, 2025

The context from CH:

Problem: In rare circumstances, the ClickHouse client connector could encounter a 'MEMORY_LIMIT_EXCEEDED' exception, the driver would fail to recognize this as an error. If this undetected exception occurred in the context of an 'INSERT' statement, the insert would have been considered successful even though no data was inserted, resulting in data loss.

Impact: Services relying on the status code from long running INSERT statements could potentially be missing some of the expected data in the table from such INSERTs if they encountered this condition.

Cause: The problem is initiated by the 'send_progress_in_http_headers=1' setting configured on the ClickHouse service, which sends a 200 status code to the client along with an error code in the header, misleading the client to interpret that the operation was successful. Versions of clickhouse-connect prior to 0.8.4 are affected by this behavior.

Fix: We've updated the clickhouse-connect client to check for the error code header, allowing for proper exception detection. The new version is available now via the pypi repository: https://pypi.org/project/clickhouse-connect and should be upgraded via 'pip install –upgrade clickhouse-connect'

Here is a query you can use to identify the impacted queries:

SELECT event_date, event_time, query_id, written_rows, query_kind, databases, tables, http_user_agent, exception_code, exception
FROM clusterAllReplicas(default, merge('system','^query_log'))
WHERE (http_user_agent LIKE 'clickhouse-connect%')
AND (exception_code = 241)
AND query ILIKE '%INSERT%'
ORDER BY event_time ASC
FORMAT PrettyCompactMonoBlock;

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants