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

Proposed implementation of options-passing #16

Merged
merged 21 commits into from
Nov 18, 2011
Merged

Conversation

ctavan
Copy link
Member

@ctavan ctavan commented Nov 18, 2011

The commits implement the ideas from #12.

So does it bloat the code too much?

- Allows to overwrite the random numbers being used
- Allows the output format to be specified
- Backwards compatible to old function-signature
- A millisecond timestamp can be provided in the options object that is used instead of Date().getTime() if present
- Sorting v1 UUIDs one has to sort by time_hi_and_version first, then time_mid, then time_low
- Add test for setting the timestamp to some time in the past
- This allows to manually overwrite the internal count of UUIDs generated in the current ms interval
- Needed to get the lowest and highest possible UUID for a given ms timestamp
var id = ids[i].split('-');
id = [id[2], id[1], id[0], id[3], id[4]].join('');
ids[i] = id;
}
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, guess my previous comment about temporal-lexical sort order wasn't quite right. Looking at the field order:

time-low-time-mid-time-high-and-version-clock-seq-and-reserved clock-seq-low-node

it appears the fields are set so the most dynamic ones appear first in the string. (Perhaps to improve string comparison performance? If two ids differ, they're likely going to differ early in the string, meaning strcmp-like tests will fail fast.) . Within a given JS context node will never change, while time-low changes every msec. So can't we just reverse the fields? I.e.:

ids = ids.map(function(id) {return ids.split('-').reverse().join('-')}).sort();

Note there's a good chance I'm just trying to find meaning where none is intended with this, btw. There is one comment in the RFC code samples that says, "Note that lexical ordering is not temporal ordering!" :P So I don't know how much value this test actually has.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think reversing won't help, since clock-seq might have been incremented and then ordering the reversed array will break.

We want to assure time-ordering for v1 UUIDs, that's all. So basically we could or maybe should just strip the clock-seq and node fields. So if we want to determine time-ordering, it will always just work up to 100ns-scale. IMO there is no reasonable choice of ordering two UUIDs that have been created at the same 100-nanosecond-tick, they will just sort randomly. The RFC just implements lexicographic ordering.

However, have a look on how cassandra determines ordering of their TimeUUIDType (which are exactly v1 UUIDs): https://github.com/apache/cassandra/blob/trunk/src/java/org/apache/cassandra/db/marshal/TimeUUIDType.java#L71

They also compare just the time bits...

@broofa
Copy link
Member

broofa commented Nov 18, 2011

I was wondering whether we should export parse, unparse and BufferClass also if people include [just uuid.v4]

You mean hang those properties off v4 and v1 as well? APIs should be as compact as possible. Having multiple references to the same state hung in different places feels messy to me.

@ctavan
Copy link
Member Author

ctavan commented Nov 18, 2011

Yeah, I didn't like it neither that's why I didn't do it :)

Yet I think the documentation is a bit confusing then... I'll try to clean that up a bit.

@broofa
Copy link
Member

broofa commented Nov 18, 2011

@pnegri : regarding performance. I agree node-uuid probably puts too much emphasis on this. After all, we're getting close to the theoreticaly limit of the RFC. It says if you're creating more than 10M uuids/sec you should throw an error or (gag!) have the code block to slow things down.

@ctavan
Copy link
Member Author

ctavan commented Nov 18, 2011

OK, I'm fine with the module as it is right now.

@broofa
Copy link
Member

broofa commented Nov 18, 2011

Yeah, I think we're in good shape here. I'm in the middle of doing some housekeeping and removing some of the unnecessary perf optimizations I had (in favor of making the code more compact/readable). I'll resolve conflicts on my end though.

broofa added a commit that referenced this pull request Nov 18, 2011
Proposed implementation of options-passing
@broofa broofa merged commit 890d312 into uuidjs:master Nov 18, 2011
@ctavan
Copy link
Member Author

ctavan commented Nov 18, 2011

Cool, thanks for all the help! I'm looking forward to seeing it published in npm...

Now I can head on to the next task, i.e. integrating node-uuid with cassandra-client, see http://code.google.com/a/apache-extras.org/p/cassandra-node/issues/detail?id=12

:)

qgerome pushed a commit to qgerome/uuid that referenced this pull request May 27, 2020
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.

None yet

2 participants