Skip to content

Switch to github.com/google/uuid#4132

Merged
milosgajdos merged 1 commit intodistribution:mainfrom
Jamstah:uuid-replacement
Oct 26, 2023
Merged

Switch to github.com/google/uuid#4132
milosgajdos merged 1 commit intodistribution:mainfrom
Jamstah:uuid-replacement

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.

3 participants