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

Replace go.uber.org/atomic with sync/atomic #910

Open
wants to merge 4 commits into
base: dev
Choose a base branch
from
Open

Conversation

serprex
Copy link

@serprex serprex commented Jan 10, 2024

go 1.19 introduced atomic types
https://tip.golang.org/doc/go1.19#atomic_types

Indirect dependency remains through multierr,
but multierr recently dropped its go.uber.org/atomic dependency: uber-go/multierr#72

@CLAassistant
Copy link

CLAassistant commented Jan 10, 2024

CLA assistant check
All committers have signed the CLA.

Copy link

@rabbbit rabbbit left a comment

Choose a reason for hiding this comment

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

Pending nits

@@ -675,7 +675,7 @@ func (r *Relayer) finishRelayItem(items *relayItems, id uint32) {
}

func (r *Relayer) decrementPending() {
r.pending.Dec()
r.pending.Add(^uint32(0))
Copy link

Choose a reason for hiding this comment

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

Suggested change
r.pending.Add(^uint32(0))
r.pending.Add(-1)

right?

Copy link
Author

@serprex serprex Jan 11, 2024

Choose a reason for hiding this comment

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

Unfortunately pending is unsigned, & there's no atomic subtract method, so this is the documented way: https://pkg.go.dev/sync/atomic#AddUint32

Added comment linking to docs

connection.go Outdated Show resolved Hide resolved
relay_test.go Outdated Show resolved Hide resolved
@@ -69,13 +69,14 @@ func TestTracingSpanAttributes(t *testing.T) {
customAppHeaderKey = "futurama"
customAppHeaderExpectedValue = "simpsons"
)
var customAppHeaderValue atomic.String
var customAppHeaderValue atomic.Pointer[string]
Copy link

Choose a reason for hiding this comment

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

a pity they don't just support strings.

Copy link
Author

@serprex serprex Jan 11, 2024

Choose a reason for hiding this comment

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

Or at least a generic Value[T]. Since these strings are only used in tests I opted not to create a utility struct wrapping atomic.Value

@serprex serprex requested a review from rabbbit January 11, 2024 15:36
@rabbbit
Copy link

rabbbit commented Jan 12, 2024

thanks for the contribution!

Out of curiosity, are you using this library anywhere?

@serprex
Copy link
Author

serprex commented Jan 12, 2024

It's a transitive dependency through https://temporal.io, which https://peerdb.io (data movement product) is built on top of
Created temporalio/temporal#5278 & temporalio/sdk-go#1344 alongside this to hopefully get one line removed from our go.mod eventually

@serprex
Copy link
Author

serprex commented Mar 27, 2024

@rabbbit who's the decision maker for merging?

@cinchurge
Copy link
Collaborator

@rabbbit who's the decision maker for merging?

sorry for the lack of movement on this. the service mesh team at Uber is currently the de facto maintainers of this repo. can you shed light on how you're using tchannel-go?

@serprex
Copy link
Author

serprex commented Jun 27, 2024

@rabbbit who's the decision maker for merging?

sorry for the lack of movement on this. the service mesh team at Uber is currently the de facto maintainers of this repo. can you shed light on how you're using tchannel-go?

rely on it via temporal

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants