[ClickHouse-58380] Implement Retries in distributed queries when server stopped during queries by Zawa-ll · Pull Request #66791 · ClickHouse/ClickHouse

Zawa-ll · 2024-07-19T22:32:35Z

Changelog category (leave one):

New Feature

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Add support for dynamic replica reconnection in the Cluster class, enabling automatic reconnection to available replicas if some replicas become unavailable during query execution.

Documentation entry for user-facing changes

Documentation is written (mandatory for new features)

Documentation Information:

Motivation: Ensures that queries can continue execution by reconnecting to other available replicas if some replicas become unavailable, improving the resilience of the cluster.
Parameters:
- Cluster::reconnectToReplica: Method to attempt reconnection to a replica if it is not connected.
- Cluster::handleDynamicReplicas: Method to handle the dynamic reconnection logic for replicas.

Information about CI checks: https://clickhouse.com/docs/en/development/continuous-integration/

CI Settings (Only check the boxes if you know what you are doing):

Allow: All Required Checks
Allow: Stateless tests
Allow: Stateful tests
Allow: Integration Tests
Allow: Performance tests
Allow: All Builds
Allow: batch 1, 2 for multi-batch jobs
Allow: batch 3, 4, 5, 6 for multi-batch jobs

Exclude: Style check
Exclude: Fast test
Exclude: All with ASAN
Exclude: All with TSAN, MSAN, UBSAN, Coverage
Exclude: All with aarch64, release, debug

This addresses issue #58380.

Zawa-ll · 2024-07-19T22:42:04Z

This ticket is involve multiple files, I will continue reviewing and working on my changes to ensure that it works as expected, feel free to review and point out mistakes I made!

robot-ch-test-poll1 · 2024-07-20T00:11:26Z

This is an automated comment for commit 92ad740 with description of existing statuses. It's updated for the latest CI running

❌ Click here to open a full report in a separate page

Check name	Description	Status
Style check	Runs a set of checks to keep the code style clean. If some of tests failed, see the related log from the report	❌ failure

Successful checks

Check name	Description	Status
Docs check	Builds and tests the documentation	✅ success

Zawa-ll · 2024-07-26T03:45:16Z

@alexey-milovidov
Hi I just notice that the current Fast Check Cannot be passed, and error like

2024-07-26 00:28:45 Connection error, will retry:  [Errno 111] Connection refused
2024-07-26 00:28:56 Connection error, will retry:  [Errno 111] Connection refused

Can be seen, can you please take a look and provide me with some help on how to fix this one? Appreciate it!

alexey-milovidov · 2024-07-26T10:36:09Z

It is a problem with your changes. Read the logs here: https://s3.amazonaws.com/clickhouse-test-reports/66791/b4511ab45e7c3e306052d5eeabdf15749b0b67d1/fast_test.html

Zawa-ll · 2024-07-26T21:56:37Z

It is a problem with your changes. Read the logs here: https://s3.amazonaws.com/clickhouse-test-reports/66791/b4511ab45e7c3e306052d5eeabdf15749b0b67d1/fast_test.html

@alexey-milovidov

Hi I read through the log you shared with me, and I noticed that it is the same as the link I checked previously: https://github.com/ClickHouse/ClickHouse/actions/runs/10101883167/job/27936478262?pr=66791
To ensure that I didn't miss anything important, I have carefully checked the log again, and I don't think I could find the error you mentioned...

Can you please share with me where the error is, or tell me which part of the log should I check? I will be really appreciate it!

alexey-milovidov · 2024-07-26T23:33:00Z

The server does not start: https://s3.amazonaws.com/clickhouse-test-reports/66791/b4511ab45e7c3e306052d5eeabdf15749b0b67d1/fast_test/run.log

Zawa-ll · 2024-07-27T04:52:46Z

The server does not start: https://s3.amazonaws.com/clickhouse-test-reports/66791/b4511ab45e7c3e306052d5eeabdf15749b0b67d1/fast_test/run.log

@alexey-milovidov
Hi sorry for the confusion, my question here is what could be the possible cause of this issue? Or which part is responsible for the logic of starting the server? I was trying to restore my changes in ConnectionPoolWithFailover.cpp and Cluster.cpp but it seems that my changes ain't work out...
I will be really appreciate it if you can provide me with some direction on which part of code should I be focusing on!

alexey-milovidov · 2024-07-27T14:59:29Z

Logs show segmentation fault at startup: https://s3.amazonaws.com/clickhouse-test-reports/66791/32e22049ce4967a220ae832a9d0cba889a3522e6/fast_test/server.log which is a null pointer dereference (for reading).

Zawa-ll · 2024-07-29T07:01:22Z

Hi @alexey-milovidov,
I've been working on fixing the issues related to connection timeouts and keep-alive settings in, I tried to rewrite my implemenation to fix the bug, but I encountered some error that really stuck me for a few days. It would be really helpful if you could kindly in reviewing my changes.

Here’s a quick summary of what I’ve done:

ConnectionPoolWithFailover.cpp:
Function Modified: ConnectionPoolWithFailover::get
What I Did: I updated the lambda function inside the get method to properly set socket timeouts and enable keep-alive on the connections. The lambda now returns a TryResult that fits the expected TryGetEntryFunc type.
Cluster.cpp:
Adjustments: I tweaked the for-loop that iterates over shards_info to make sure it correctly gets connections and sets their socket timeouts and keep-alive settings.
ConnectionTimeouts.h:
Changes: I added a constructor to the ConnectionTimeouts struct so it can take multiple Poco::Timespan parameters. This helps with initializing the timeouts properly.
These changes should ensure the connections have the right timeouts and keep-alive settings, and the lambda function now matches the expected type.

Despite refactoring my code based on the log you shared with me, I still encounter the following error:
Code: 210. DB::NetException: Connection refused (localhost:9000). (NETWORK_ERROR)

I wasn't sure if I am changing the correct file, or did I over complicate the task...

It would be incredibly helpful if you could provide me with any help or point out any mistakes so I don't go too far down the wrong path.
Thanks a lot if you could do me a favor, I will be really appreciate that

update: Some new changes were implemented and it seems to work well, I will apply testing on it

…er stopped during queries

Removed unnecessary try-catch Add Comments back Style check

…lientSession.cpp and SessionImpl.cpp

Use Existing logic from other files Added missing right bracket Added Missing param Fixed Cluster.cpp Fixed Cluster.h Fixed Cluster.cpp Fixed Cluster.h again Create an public getter for getEntry() handled unused e modify head file of ConnectPoolwithfailover handled header

…ibuted.h

… Setting.h

…mat and SinkToStorage

…stributed-queries

…distributed-queries

Zawa-ll · 2024-08-23T22:38:29Z

Hi @alexey-milovidov @devcrafter! Thanks for taking care of this PR,
I tried to rebase the current PR but it has the previous base with flaky tests issue that cannot be passed...

I just create a new rebased PR from with the same changes Ive made from this one, could you please check that one (#68581) instead? Appreciate it!!

…stributed-queries

devcrafter · 2024-08-27T18:50:47Z

Hi @alexey-milovidov @devcrafter! Thanks for taking care of this PR, I tried to rebase the current PR but it has the previous base with flaky tests issue that cannot be passed...

I just create a new rebased PR from with the same changes Ive made from this one, could you please check that one (#68581) instead? Appreciate it!!

There is no need to create new PR.
You've just messed up things with 02572_query_views_log_background_thread. Also, if you add new settings, it's necessary to add it to settings changes history.

devcrafter

AFAIS, the PR is changing code related to distributed INSERT which unrelated to distributed SELECT as should be per initial task #58380 . What are you trying to implement?

…ELECT instead of SELECT

Zawa-ll · 2024-09-15T04:12:21Z

@devcrafter Thank you for taking care of this issue, I will fix the code based on the comments, thank you!!

clickhouse-gh · 2024-10-29T13:11:55Z

Dear @devcrafter, this PR hasn't been updated for a while. You will be unassigned. Will you continue working on it? If so, please feel free to reassign yourself.

alexey-milovidov added the can be tested Allows running workflows for external contributors label Jul 20, 2024

robot-ch-test-poll1 added the pr-feature Pull request with new product feature label Jul 20, 2024

Zawa-ll force-pushed the 58380-retries-in-distributed-queries branch 4 times, most recently from 86cd55f to 5a762ed Compare July 24, 2024 04:46

Resolved Conflict by rebase

aced858

Zawa-ll force-pushed the 58380-retries-in-distributed-queries branch from 6ccf304 to 2aeb470 Compare July 24, 2024 17:24

Zawa-ll force-pushed the 58380-retries-in-distributed-queries branch from 21b0d1f to 32e2204 Compare July 27, 2024 17:44

Zawa-ll added 12 commits July 31, 2024 14:37

Resolved merge conflicts in RemoteQueryExecutor files

f9cb6c7

[ClickHouse-58380] Implement Retries in distributed queries when serv…

18ba382

…er stopped during queries

Removed unnecessary try-catch

356e24f

Removed unnecessary try-catch Add Comments back Style check

Update logic for reconnect from ConnectionPoolWithFailover.cpp, HTTPC…

79feef7

…lientSession.cpp and SessionImpl.cpp

Use std for writing Cluster.h and Cluster.cpp

e3dca55

Move the definition of ReplicaState struct to the top of StorageDistr…

8e3705f

…ibuted.h

Fix Param order error in DistributedSink.cpp and Added max_retry into…

9e71231

… Setting.h

Change the order of properties in DistributedSink.h

218d065

Change ping to pong

c6da674

Change .isComplete() to .complete in QueryPipeline.h

c39a9e6

Removed unnecessary reconnect() and isConnectionAlive() in IOutputFor…

92366bf

…mat and SinkToStorage

Added missing empty line at the end

cff1530

Zawa-ll mentioned this pull request Aug 4, 2024

Test Failures in 01624_soft_constraints and 02944_variant_as_common_type #67769

Closed

Fixed error syntax in 02572_query_view_log_background_thread.sh

552b4b0

Zawa-ll force-pushed the 58380-retries-in-distributed-queries branch from f92401b to 552b4b0 Compare August 7, 2024 02:27

Merge remote-tracking branch 'upstream/master'

89dc74a

devcrafter self-assigned this Aug 13, 2024

Merge remote-tracking branch 'origin/master' into 58380-retries-in-di…

0bad5ba

…stributed-queries

Zawa-ll force-pushed the 58380-retries-in-distributed-queries branch from 439a1cd to 0bad5ba Compare August 18, 2024 04:59

Zawa-ll added 4 commits August 18, 2024 21:22

Fixed Rebase error

d7ab6f5

Restore wrong rebase in DistributedSink.h

df9e5d9

Put specification of reconnect And Resend to the right place

6963fe4

Merge remote-tracking branch 'upstream/master' into 58380-retries-in-…

f2ce342

…distributed-queries

Zawa-ll mentioned this pull request Aug 19, 2024

REBASED VERSION - [ClickHouse-58380] Implement Retries in distributed queries when server stopped during queries #68581

Closed

19 tasks

devcrafter added 3 commits August 27, 2024 15:40

Merge remote-tracking branch 'origin/master' into 58380-retries-in-di…

398d3b1

…stributed-queries

Fix tests

0a64c95

Fix 02995_new_settings_history

0136b4a

devcrafter reviewed Aug 27, 2024

View reviewed changes

Zawa-ll and others added 7 commits August 30, 2024 20:04

Restore changes in DistributedSink.cpp and make changes relevant to S…

b8e812a

…ELECT instead of SELECT

Removed Unnessary chagnes in DistributedSink.cpp

a235727

Added Missing include header in DistributedSink.cpp

ad22826

Resolved Potential Conflict

ad9e40a

Resolve again

bb63a65

Merge branch 'master' into 58380-retries-in-distributed-queries

d3e22b2

Added missing Error type specification

c188700

Zawa-ll force-pushed the 58380-retries-in-distributed-queries branch from 92ad740 to c188700 Compare September 24, 2024 04:56

clickhouse-gh bot unassigned devcrafter Oct 29, 2024

Conversation

Zawa-ll commented Jul 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Changelog category (leave one):

Changelog entry (a user-readable short description of the changes that goes to CHANGELOG.md):

Documentation entry for user-facing changes

CI Settings (Only check the boxes if you know what you are doing):

Uh oh!

Zawa-ll commented Jul 19, 2024

Uh oh!

robot-ch-test-poll1 commented Jul 20, 2024 • edited by robot-ch-test-poll2 Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Zawa-ll commented Jul 26, 2024

Uh oh!

alexey-milovidov commented Jul 26, 2024

Uh oh!

Zawa-ll commented Jul 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

alexey-milovidov commented Jul 26, 2024

Uh oh!

Zawa-ll commented Jul 27, 2024

Uh oh!

alexey-milovidov commented Jul 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Zawa-ll commented Jul 29, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Zawa-ll commented Aug 23, 2024

Uh oh!

devcrafter commented Aug 27, 2024

Uh oh!

devcrafter left a comment

Choose a reason for hiding this comment

Uh oh!

Zawa-ll commented Sep 15, 2024

Uh oh!

clickhouse-gh bot commented Oct 29, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Zawa-ll commented Jul 19, 2024 •

edited

Loading

robot-ch-test-poll1 commented Jul 20, 2024 •

edited by robot-ch-test-poll2

Loading

Zawa-ll commented Jul 26, 2024 •

edited

Loading

alexey-milovidov commented Jul 27, 2024 •

edited

Loading

Zawa-ll commented Jul 29, 2024 •

edited

Loading