chore: refactor to use modern atomic types - #3277
Merged
Merged
Conversation
Sahil-4555
force-pushed
the
refactor-atomic-types
branch
from
September 2, 2025 05:59
198bd93 to
a36a9b8
Compare
puellanivis
reviewed
Sep 2, 2025
puellanivis
left a comment
puellanivis
left a comment
Collaborator
There was a problem hiding this comment.
Just a few suggestions.
Signed-off-by: Sahil-4555 <sahilsojitra4555@gmail.com>
Signed-off-by: Sahil-4555 <sahilsojitra4555@gmail.com>
Sahil-4555
force-pushed
the
refactor-atomic-types
branch
from
September 2, 2025 14:49
659dc7a to
32abe92
Compare
Signed-off-by: Sahil-4555 <sahilsojitra4555@gmail.com>
Sahil-4555
force-pushed
the
refactor-atomic-types
branch
from
September 2, 2025 14:59
aaab49a to
2fb0acd
Compare
puellanivis
reviewed
Sep 2, 2025
puellanivis
left a comment
puellanivis
left a comment
Collaborator
There was a problem hiding this comment.
I don’t see anything to comment on. 👍
dnwe
approved these changes
Sep 2, 2025
dnwe
left a comment
dnwe
left a comment
Collaborator
There was a problem hiding this comment.
Thanks! A good piece of housekeeping
dnwe
pushed a commit
that referenced
this pull request
Dec 17, 2025
This completes the change in #3277 and rolls out all atomics usage to using the types, rather than the bare functions. This already found a few usages that had inconsistently used atomic functions on some values. --------- Signed-off-by: Cassondra Foesch <puellanivis@gmail.com>
3AceShowHand
pushed a commit
to 3AceShowHand/sarama
that referenced
this pull request
May 7, 2026
3AceShowHand
added a commit
to pingcap/sarama
that referenced
this pull request
May 7, 2026
* fix: close broken tcp connections Prevent TCP connection leaks that error occurred to kernel space's socket. Previously, when errors occurred on TCP sockets, connections would remain in TCP_CLOSE state indefinitely without being properly freed. The leaked connections caused a cascade of kernel-level issues: - TCP sockets remain in TCP_CLOSE state indefinitely - Their sk_receive_queues retain FINACK skb packets - These skbs hold pages allocated from page_pool - page_pool_release_retry() stalls because pages cannot be freed while still referenced by the TCP stack The fix ensures that TCP connections are properly closed when socket errors occur, preventing resource leaks at both the application and kernel levels. Signed-off-by: Leon Hwang <leon.hwang@linux.dev> * fix: keep bootstrap brokers and unblock async shutdown Skip socket health checks while a broker is still opening so metadata refresh does not block behind the broker mutex. Keep seed and dead-seed brokers in their bootstrap pools after socket errors so they can be reopened on a later refresh. Close broker sockets before waiting for the response reader so health-check cleanup and SASL v1 auth failures cannot hang while async responses are still in flight. Signed-off-by: Dominic Evans <dominic.evans@uk.ibm.com> * fix: skip broker health tests without socket probing Signed-off-by: Dominic Evans <dominic.evans@uk.ibm.com> * fix: remove return from checkSeedBrokersHealth Signed-off-by: Dominic Evans <dominic.evans@uk.ibm.com> * fix: add Unwrap() to DescribeConfigError and AlterConfigError (IBM#3487) ## Summary `TopicError` and `TopicPartitionError` implement `Unwrap()` returning their `KError` field, enabling `errors.Is`/`errors.As` to traverse the error chain. However, `DescribeConfigError` and `AlterConfigError` have the same structure (`Err KError` + `ErrMsg`) but were missing `Unwrap()`, making `errors.Is`/`errors.As` unable to match the underlying `KError`. This adds the missing `Unwrap()` methods for consistency, allowing callers to use `errors.Is(err, sarama.ErrInvalidConfig)` instead of type-asserting to extract the error code. ## Changes - Add `Unwrap() error` to `*DescribeConfigError` (`describe_configs_response.go`) - Add `Unwrap() error` to `*AlterConfigError` (`alter_configs_response.go`) - Add tests for both, following the existing `TestTopicError` pattern ## Test plan - [x] `go test -run 'TestDescribeConfigError|TestAlterConfigError|TestTopicError'` — all pass - [x] `go vet ./...` — clean Signed-off-by: Lingnan Liu <xmxu00@gmail.com> * fix conflicts * fix conflicts * chore: refactor to use modern atomic types (IBM#3277) --------- Signed-off-by: Leon Hwang <leon.hwang@linux.dev> Signed-off-by: Dominic Evans <dominic.evans@uk.ibm.com> Signed-off-by: Lingnan Liu <xmxu00@gmail.com> Co-authored-by: Leon Hwang <leon.hwang@linux.dev> Co-authored-by: Dominic Evans <dominic.evans@uk.ibm.com> Co-authored-by: Lingnan Liu <xmxu00@gmail.com> Co-authored-by: Sahil Sojitra <88416181+Sahil-4555@users.noreply.github.com>
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
3 participants
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.
What type of PR is this?
What does this PR do? Why is it needed?
This PR updates our atomic operations to use
Go 1.19'snewer typed atomic approach instead of the old function-based method. We're changing fields likeint64toatomic.Int64so we can write cleaner code likechild.retries.Add(1)instead ofatomic.AddInt32(&child.retries, 1). This removes the need for pointer handling and gives us better compile-time safety when multiple threads access the same variables in operations.Which issues(s) does this PR fix?
The function-based atomics allowed accidental non-atomic access to shared variables, causing race conditions in concurrent operations. The verbose
atomic.AddInt32(&child.retries, 1)syntax was harder to maintain. Typed atomics enforce atomic-only access at compile time and use cleaner method calls, preventing threading bugs.