Use v4 instead of v1 uuids#5235
Merged
chnn merged 2 commits intoinfluxdata:masterfrom Jul 28, 2019
Merged
Conversation
This uses a purely random v4 UUID instead of a time-based v1 UUID for the worker message IDs (which were introduced in 1a76a29). v1 UUID are based on current time and the hardware MAC address of the machine where they are being generated (although the implementation in the npm uuid module uses generates a random fake MAC address). As such they have much more complex semantics than v4 UUIDs which are simply randomly generated. Unless there's a specific requirement for the special semantics of v1 UUIDs it is simpler and less error prone to simply go for v4 UUIDs whenever just a unique identifier is needed.
This uses a purely random v4 UUID instead of a time-based v1 UUID for the time series UUIDs (which were introduced in dba9e28 and 2fba2b9). As far as I can tell from https://github.com/influxdata/chronograf/blob/b6f6118b65b9a6ba5877e00dc98bab19f80a224f/server/influx.go#L91-L94 the server uses https://github.com/influxdata/chronograf/blob/b6f6118b65b9a6ba5877e00dc98bab19f80a224f/id/uuid.go#L15 to generate the `response.uuid` field where clearly a v4 UUID is being generated. Mixing this with v1 UUIDs in the client code seems odd. v1 UUID are based on current time and the hardware MAC address of the machine where they are being generated (although the implementation in the npm uuid module uses generates a random fake MAC address). As such they have much more complex semantics than v4 UUIDs which are simply randomly generated. Unless there's a specific requirement for the special semantics of v1 UUIDs it is simpler and less error prone to simply go for v4 UUIDs whenever just a unique identifier is needed.
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.
Mostly copied over from influxdata/influxdb#14374
I'm a co-author of the uuid npm module which is being used in some places within the code base of influxdb.
I'm currently trying to understand real-world use cases of time-based UUIDs ("v1 UUIDs").
I found two occurrence where
v1UUIDs were being used instead ofv4UUIDs and I was wondering if this case really requires the semantically much more complexv1UUIDs or whether we could live equally well with purely randomv4UUIDs? Please see my commit messages for more details.At least the test suite still passes after my changes, so apparently this doesn't seem to introduce any known regressions.
I'd be really curious to understand the motivation for choosing
v1overv4UUIDs in the first place, so any feedback on this would be highly appreciated!@bthesorceror as far as I can tell from the git history it seems like you have introduced both usages of
v1UUIDs so I'd be particularly interested in your feedback.