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.

Sign up for GitHub

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

Jump to bottom

deps: Consider replacing UUID package #26332

Closed
bdarnell opened this issue Jun 1, 2018 · 6 comments
Closed

deps: Consider replacing UUID package #26332

bdarnell opened this issue Jun 1, 2018 · 6 comments
Labels
A-kv-client Relating to the KV client and the KV interface. A-sql-datatypes SQL column types usable in table descriptors. C-investigation Further steps needed to qualify. C-label will change.
Milestone
Later

Comments

@bdarnell
Copy link
Contributor

bdarnell commented Jun 1, 2018

We currently use version 1.2.0 of github.com/satori/go.uuid. The master branch of that repo contains backwards-incompatible API changes and a major regression that results in UUIDs being partially filled with zeros (satori/go.uuid#73). The maintainer has been unresponsive (PR satori/go.uuid#75 to fix the bug has been open with no response for nearly two months now). It may be time to find a new UUID library.
@knz
Copy link
Contributor

knz commented Jun 6, 2018

My personal suggestion would be to drop the uuid package entirely and just use [16]byte in cockroachdb. We know how to use rand.Reader.

@knz
Copy link
Contributor

knz commented Jun 6, 2018

We also need to be on the lookout for well-intentioned people on the team who might attempt to refresh our deps. How can we make a blinking neon light warning to not upgrade this package? @benesch thoughts?

@knz knz added C-investigation Further steps needed to qualify. C-label will change. A-kv-client Relating to the KV client and the KV interface. A-sql-datatypes SQL column types usable in table descriptors. labels Jun 6, 2018
@bdarnell
Copy link
Contributor Author

bdarnell commented Jun 6, 2018

Yeah, as long as we only need v4 UUIDs (and I see no reason to support the other versions), it's simple enough that I see no need for an external dependency.

How can we make a blinking neon light warning to not upgrade this package?

A pin with a comment in Gopkg.toml should be sufficient.

bdarnell added a commit to bdarnell/cockroach that referenced this issue Jun 6, 2018
@bdarnell
build: Pin go.uuid to the version currently in use
b09ab16 
Updates cockroachdb#26332

Release note: None
@bdarnell bdarnell mentioned this issue Jun 6, 2018
Merged
craig bot pushed a commit that referenced this issue Jun 6, 2018
@asubiotto @bdarnell
Merge #26441 #26463 #26469 #26470
f6f6ee8 
26441: distsql: add NewFinalIterator to the rowIterator interface r=asubiotto a=asubiotto

Some implementations of the rowIterator interface would destroy rows as
they were iterated over to free memory eagerly. NewFinalIterator is
introduced in this change to provide non-reusable behavior and
NewIterator is explicitly described as reusable.

A reusable iterator has been added to the memRowContainer to satisfy
these new interface semantics.

Release note: None

26463: storage: Disable campaign-on-wake when receiving raft messages r=bdarnell a=bdarnell

When the incoming message is a MsgVote (which is likely the case for
the first message received by a quiesced follower), immediate
campaigning will cause the election to fail.

This is similar to reverting commit 44e3977, but only disables
campaigning in one location.

Fixes #26391

Release note: None

26469: lint: Fix a file descriptor leak r=bdarnell a=bdarnell

This leak is enough to cause `make lintshort` fail when run under the
default file descriptor limit on macos (256).

Release note: None

26470: build: Pin go.uuid to the version currently in use r=bdarnell a=bdarnell

Updates #26332

Release note: None

Co-authored-by: Alfonso Subiotto Marqués <[email protected]>
Co-authored-by: Ben Darnell <[email protected]>
@gpaul
Copy link

gpaul commented Jun 6, 2018

UUID v4 has structure. Consider using google's uuid package?
https://github.com/google/uuid

@bdarnell
Copy link
Contributor Author

bdarnell commented Jun 6, 2018

That package hasn't been updated in 6 months but the first line of its readme is "This package is currently in development and the API may not be stable."

It's true that UUID v4 is not just 128 random bits, but there's not much else to it (just 6 bits that must be set to specific values). If there's a stable de-facto standard library for this I'm happy to use it, but experience suggests that we can implement this ourselves with less headache than using an external library.

@bdarnell bdarnell mentioned this issue Jun 7, 2018
Merged
@tbg tbg modified the milestones: 2.2, Later Jul 22, 2018
@nvanbenschoten nvanbenschoten mentioned this issue Sep 14, 2018
Closed
@bdarnell
Copy link
Contributor Author

bdarnell commented Dec 2, 2020

This was done in fd9705b; we no longer use a third-party dependency for UUID generation.

@bdarnell bdarnell closed this as completed Dec 2, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
A-kv-client Relating to the KV client and the KV interface. A-sql-datatypes SQL column types usable in table descriptors. C-investigation Further steps needed to qualify. C-label will change.
Projects
None yet
Milestone
Later
Development

No branches or pull requests
4 participants
@gpaul @bdarnell @knz @tbg