-
Notifications
You must be signed in to change notification settings - Fork 32
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
feat: Add Tid::now
and Tid::from_datetime
constructors
#277
base: main
Are you sure you want to change the base?
Conversation
atrium-api/src/types/string.rs
Outdated
@@ -436,6 +452,27 @@ impl Tid { | |||
} | |||
} | |||
|
|||
/// Construct a new timestamp with the specified clock ID. | |||
/// | |||
/// Clock IDs 0-31 can be used as an ad-hoc clock ID if you are not concerned |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the Clock ID 0-31? I am wondering because the specs say 10bit and it seems to be a value in the range 0-1023. If you know, I would like to know.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Here's some more info that I've found online: bluesky-social/atproto#1160 (comment)
It appears that the clock ID partitioning did not make it into the specification - but based on this, clocks 0-31 are ad-hoc identifiers (and one is randomly chosen by the reference implementation).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the reply! I don't think we need to bother writing about the 0-31
range as long as it is not explicitly stated in the specification.
It seems more important to have a mechanism to ensure that a value larger than the previously issued timestamp is generated in order to avoid collisions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good!
It seems more important to have a mechanism to ensure that a value larger than the previously issued timestamp is generated in order to avoid collisions.
Agreed, though I'm still up in the air about who should be responsible for this.
The upstream libraries written by Bluesky handle this because they have millisecond-level precision and conflicts are very likely to occur at that level.
However, they do this by maintaining global state, which is far less than ideal.
There are other factors as well - like I'm not really sure if a conflict matters if an application is writing separate records.
E.g. AFAIK, you could have a com.example.foo
and a com.example.bar
with the same record key.
I'm wondering if we should just advise application developers about this hazard and have them handle conflicts, such as repeatedly calling Tid::now()
if it returns the same value.
Or maybe provide a wrapper function or something?
Let me keep thinking on this...
@sugyan Btw sorry - I've been pretty spotty just because I'm limited on time here; I'm doing this work in my free time at this point. If you want to speed this along, absolutely feel free to push changes to my branch here. If not, I will follow-up eventually :) |
This simply adds some new
Tid
convenience constructors so that users do not have to write their own if they want to construct a timestamp.