Skip to content

Allow to use bigger timeouts for zookeeper#85008

Merged
azat merged 1 commit intoClickHouse:masterfrom
azat:zk-int64-timeouts
Aug 4, 2025
Merged

Allow to use bigger timeouts for zookeeper#85008
azat merged 1 commit intoClickHouse:masterfrom
azat:zk-int64-timeouts

Conversation

@azat
Copy link
Copy Markdown
Member

@azat azat commented Aug 4, 2025

I am using bigger timeouts for development (3600 seconds) to avoid disconnects in case of attaching with debugger, but this may trigger UB due to overflow:

src/Common/ZooKeeper/ZooKeeperImpl.cpp:556:68: runtime error: signed integer overflow: 3600000 * 1000 cannot be represented in type 'int'

Changelog category (leave one):

  • Not for changelog (changelog entry is not required)

@clickhouse-gh
Copy link
Copy Markdown
Contributor

clickhouse-gh Bot commented Aug 4, 2025

Workflow [PR], commit [e902388]

Summary:

job_name test_name status info comment
Stateless tests (amd_binary, old analyzer, s3 storage, DatabaseReplicated, parallel) failure
02443_detach_attach_partition FAIL
Stateless tests (amd_binary, old analyzer, s3 storage, DatabaseReplicated, sequential) failure
01154_move_partition_long FAIL
Integration tests (aarch64, distributed plan, 4/4) failure
test_storage_rabbitmq/test.py::test_rabbitmq_vhost FAIL
test_storage_rabbitmq/test.py::test_rabbitmq_format_factory_settings FAIL

@clickhouse-gh clickhouse-gh Bot added the pr-not-for-changelog This PR should not be mentioned in the changelog label Aug 4, 2025
tuanpach
tuanpach previously approved these changes Aug 4, 2025
@tuanpach
Copy link
Copy Markdown
Member

tuanpach commented Aug 4, 2025

I am using bigger timeouts for development (3600 seconds)
src/Common/ZooKeeper/ZooKeeperImpl.cpp:556:68: runtime error: signed integer overflow: 3600000 * 1000 cannot be represented in type 'int'

Should the value be 3600*1000 ms instead of 3600000 * 1000?

@azat
Copy link
Copy Markdown
Member Author

azat commented Aug 4, 2025

Should the value be 3600*1000 ms instead of 3600000 * 1000?

The last is correct, the value in config accept ms, I put 3600_000 in config, but later to pass timeout to socket poco converts it to microseconds, this is 1000x more, and so it exceeds INT_MAX

@antonio2368
Copy link
Copy Markdown
Member

We cannot do this because protocol defines timeout as int32 and that's what we will write and read in handshake.

@azat
Copy link
Copy Markdown
Member Author

azat commented Aug 4, 2025

We cannot do this because protocol defines timeout as int32 and that's what we will write and read in handshake.

Thanks, then, let's do it another way, @antonio2368 PTAL

@azat azat marked this pull request as draft August 4, 2025 08:45
Copy link
Copy Markdown
Member

@antonio2368 antonio2368 left a comment

Choose a reason for hiding this comment

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

This looks good
Always some issues with Poco::Timespan 😞

@azat azat force-pushed the zk-int64-timeouts branch 2 times, most recently from 7208987 to 7ce849e Compare August 4, 2025 13:52
@azat azat marked this pull request as ready for review August 4, 2025 14:39
@azat azat enabled auto-merge August 4, 2025 14:40
I am using bigger timeouts for development (3600 seconds) to avoid
disconnects in case of attaching with debugger, but this may trigger UB
due to overflow:

    src/Common/ZooKeeper/ZooKeeperImpl.cpp:556:68: runtime error: signed integer overflow: 3600000 * 1000 cannot be represented in type 'int'
@azat azat force-pushed the zk-int64-timeouts branch from 7ce849e to e902388 Compare August 4, 2025 15:00
@azat
Copy link
Copy Markdown
Member Author

azat commented Aug 4, 2025

@azat azat added this pull request to the merge queue Aug 4, 2025
Merged via the queue into ClickHouse:master with commit 43d9f05 Aug 4, 2025
121 of 124 checks passed
@azat azat deleted the zk-int64-timeouts branch August 4, 2025 20:10
@robot-ch-test-poll robot-ch-test-poll added the pr-synced-to-cloud The PR is synced to the cloud repo label Aug 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

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

Development

Successfully merging this pull request may close these issues.

4 participants