fix(Cluster): pass seconds, not milliseconds, for heartbeat and TCP keepalive - #145
Open
CodeLieutenant wants to merge 1 commit into
Open
fix(Cluster): pass seconds, not milliseconds, for heartbeat and TCP keepalive#145CodeLieutenant wants to merge 1 commit into
CodeLieutenant wants to merge 1 commit into
Conversation
…eepalive withConnectionHeartbeatInterval and withTCPKeepalive shared the seconds-to-milliseconds helper used by the timeout setters. The two cpp-driver functions behind them take seconds: cass_cluster_set_connection_heartbeat_interval(cluster, interval_secs) cass_cluster_set_tcp_keepalive(cluster, enabled, delay_secs) The result was a 1000x error. withConnectionHeartbeatInterval(30.0) sent 30000 seconds, about 8.3 hours, which disabled heartbeats. The defaults in php_scylladb_cluster_builder_new are raw seconds, so seconds was always the intended unit. Add php_scylladb_set_interval_seconds for the two settings that need seconds, and store the TCP keepalive delay in seconds. get_properties no longer divides tcpKeepalive by 1000, because the field is now seconds. The public PHP API does not change. Both methods still take seconds. Add tests/Unit/Cluster/BuilderTest.php. It checks the defaults, the seconds round-trip through the exposed connectionHeartbeatInterval and tcpKeepalive properties, the null case that disables keepalive, and the rejection of negative values. reconnectInterval is included as a control, because it stays in milliseconds internally.
|
Tick the box to add this pull request to the merge queue (same as
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Problem
withConnectionHeartbeatInterval()andwithTCPKeepalive()converted their argument from seconds to milliseconds. The two cpp-driver functions behind them take seconds:This is a 1000x error.
withConnectionHeartbeatInterval(30.0)sent 30000 seconds, about 8.3 hours, which disabled heartbeats. Dead connections then stayed in the pool until a request failed on them.The defaults in
php_scylladb_cluster_builder_neware raw seconds (connection_heartbeat_interval = 30), so seconds was always the intended unit. Only the setters were wrong.Changes
src/Cluster/Builder.c— addphp_scylladb_set_interval_seconds()next to the existing millisecond helper.withConnectionHeartbeatIntervaluses it.src/Cluster/Builder.c—withTCPKeepalivestoresceil(delay)instead ofceil(delay * 1000).src/Cluster/BuilderHandlers.c—get_propertiesno longer dividestcpKeepaliveby 1000. That divide was the mirror of the same bug, so the property read back the value the user passed in and hid the defect.The three genuine millisecond settings are untouched:
withConnectTimeout,withRequestTimeout, andwithReconnectInterval.Public API
No change. Both methods still take seconds.
Tests
New
tests/Unit/Cluster/BuilderTest.php, 13 cases:connectionHeartbeatIntervalis 30,tcpKeepaliveis nullconnectionHeartbeatIntervalandtcpKeepalivepropertieswithTCPKeepalive(null)disables keepaliveCassandra\Exception\InvalidArgumentExceptionreconnectIntervalas a control, because it stays in milliseconds internallyFull unit suite on PHP 8.5 NTS: 816 passed. One pre-existing failure in
tests/Unit/Uuid/UuidTest.php:112, a cross-process child-spawn quirk that is unrelated to this change.Note for the reviewer
Both methods take
floatbut now truncate to whole seconds throughceil, so0.5becomes1. That matches the C API, which has no sub-second granularity, but the PHP signature still advertises precision it cannot deliver. Changing the stub tointis a BC break and belongs in a separate decision.