Skip to content

feat: Add Tid::now and Tid::from_datetime constructors#277

Merged
sugyan merged 5 commits intoatrium-rs:mainfrom
DrChat:tid_now
Feb 17, 2025
Merged

feat: Add Tid::now and Tid::from_datetime constructors#277
sugyan merged 5 commits intoatrium-rs:mainfrom
DrChat:tid_now

Conversation

@DrChat
Copy link
Contributor

@DrChat DrChat commented Feb 1, 2025

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.

@sugyan sugyan self-requested a review February 3, 2025 14:28

/// 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
Copy link
Member

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.

Copy link
Contributor Author

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).

Copy link
Member

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.

Copy link
Contributor Author

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...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Could abuse the iterator API here, e.g from the user's side:

// Construct multiple timestamps. The implementation guarantees that the same timestamp will not be returned twice.
let tids = Tid::repeat().take(5).collect::<Vec<_>>();

Copy link
Member

Choose a reason for hiding this comment

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

Yes, it is. Various points may need to be considered.

If it is a function like the TypeScript library that refers to a previously issued Tid and generates new one, it may be possible to avoid the global state, but it may not be easy to use....

I think it would be OK to merge this branch as it is now, as long as the user is careful about using it. What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm cool with merging this. Let me add a note to the docs to inform users about this hazard and then it should be good to go :)

@DrChat
Copy link
Contributor Author

DrChat commented Feb 8, 2025

@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 :)

@sugyan sugyan merged commit 7830d8e into atrium-rs:main Feb 17, 2025
13 checks passed
@github-actions github-actions bot mentioned this pull request Feb 17, 2025
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.

2 participants