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

Switch to github.com/google/uuid #4132

Merged
merged 1 commit into from Oct 26, 2023

Conversation

Jamstah
Copy link
Collaborator

@Jamstah Jamstah commented Oct 25, 2023

Our own package wasn't adding anything.

Signed-off-by: James Hewitt <james.hewitt@uk.ibm.com>
Copy link
Member

@milosgajdos milosgajdos left a comment

Choose a reason for hiding this comment

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

LGTM

@milosgajdos milosgajdos added dependencies Pull requests that update a dependency file cleanup labels Oct 25, 2023
@corhere
Copy link
Collaborator

corhere commented Oct 25, 2023

Our own package wasn't adding anything.

That I'm not so sure about. Our bespoke uuid package was introduced in #556 as a replacement of the predecessor to github.com/google/uuid of all things. Apparently backoff-retry on EPERM from the random source was a motivating factor. It probably didn't help that the uuid dependency at the time did this unconditionally:

// randomBits completely fills slice b with random data.
func randomBits(b []byte) {
if _, err := io.ReadFull(rander, b); err != nil {
panic(err.Error()) // rand should never fail
}
}

The "github.com/google/uuid".NewString() function is the same: read from the random source or panic. Random-source errors clearly were a problem:

so I am concerned that just doing the equivalent of reverting #556 is going to introduce regressions. Could we wrap google/uuid in the retry logic to retain the existing behaviour?

@milosgajdos
Copy link
Member

I vaguely remember reading through that issue. I also found this comment by Stevoo kinda unresolved:

#782 (comment)

Specifically, we never learnt what the real error actually was 🤷‍♂️

@Jamstah
Copy link
Collaborator Author

Jamstah commented Oct 26, 2023

so I am concerned that just doing the equivalent of reverting #556 is going to introduce regressions. Could we wrap google/uuid in the retry logic to retain the existing behaviour?

We could, but that was back in 2015 and I expect there have been many improvements since then.

Most of the places we create a uuid we don't have err in the return, so we couldn't fail more cleanly than panicking too.

Without a good understanding of why the workaround was needed, I think its sensible to remove it and see if we get any tickets during our beta period.

Just for fun, I created a billion uuids on my laptop using the google library. It took 5m39s and didn't panic once. I know that doesn't recreate whatever issue we had before, but I thought I'd try it.

Copy link
Collaborator

@corhere corhere left a comment

Choose a reason for hiding this comment

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

Fair enough; I'm okay with scream-testing if you are

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup dependencies Pull requests that update a dependency file
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants