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

WIP: 64bit nanosecond jmapids #5177

Draft
wants to merge 14 commits into
base: master
Choose a base branch
from

Conversation

ksmurchison
Copy link
Contributor

@ksmurchison ksmurchison commented Dec 19, 2024

TODO:

  • Increment nanoseconds if need be to avoid ID conflicts
  • Handle replication between different versions. How do we update a replica with the nanosecond internaldate and basecid?
  • Reconstruct cyrus.index and conv.db with nanosecond internaldates and basecid
  • ???

@ksmurchison ksmurchison marked this pull request as draft December 19, 2024 15:21
@ksmurchison ksmurchison force-pushed the 64bit-nanosecond-jmapids branch from be60440 to 5599d95 Compare December 20, 2024 18:43
@ksmurchison ksmurchison force-pushed the 64bit-nanosecond-jmapids branch 2 times, most recently from 63fc02b to 4fb40bd Compare January 6, 2025 14:15
@ksmurchison ksmurchison requested a review from rsto January 6, 2025 14:16
@rsto rsto removed their request for review January 8, 2025 08:00
@ksmurchison ksmurchison force-pushed the 64bit-nanosecond-jmapids branch 5 times, most recently from 5d8190f to 2e5b103 Compare January 15, 2025 19:12
@ksmurchison ksmurchison force-pushed the 64bit-nanosecond-jmapids branch 9 times, most recently from 33bef57 to deb2012 Compare January 22, 2025 17:06
@ksmurchison ksmurchison force-pushed the 64bit-nanosecond-jmapids branch from deb2012 to 8141e3d Compare January 23, 2025 19:44
@ksmurchison ksmurchison requested review from brong, rsto and elliefm January 26, 2025 20:16
@ksmurchison
Copy link
Contributor Author

ksmurchison commented Jan 26, 2025

This isn't complete yet but could probably use a sanity check. Reviewing commit-by-commit would probably be the easiest.

@rsto
Copy link
Member

rsto commented Jan 27, 2025

@ksmurchison since this is a WIP review, could you please let me know the main file or functional areas you would like me to sanity check?

@ksmurchison
Copy link
Contributor Author

@ksmurchison since this is a WIP review, could you please let me know the main file or functional areas you would like me to sanity check?

You should probably look at conversations.c and jmap_mail.c

@rsto
Copy link
Member

rsto commented Jan 27, 2025

You should probably look at conversations.c and jmap_mail.c

Thanks, will do.

Copy link
Member

@rsto rsto left a comment

Choose a reason for hiding this comment

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

I gave conversations.c and jmap_mail.c a read. It looks to implement the requirements, I just have a question about refcounts as well as some thoughts about function names.

imap/conversations.h Outdated Show resolved Hide resolved
imap/conversations.c Outdated Show resolved Hide resolved
const char *data, *p;
int r;

if (strlen(jidrep) != CONV_JMAPID_SIZE)
Copy link
Member

Choose a reason for hiding this comment

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

Should this better return an internal error? Or is there any scenario where the caller might legitimately pass a jidrep that doesn't match the expected size?

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 guess the JMAP code should be checking the id early, and this function can assume that its well-formed.

Should I remove this check?

Copy link
Member

Choose a reason for hiding this comment

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

I would rather keep that check but either return CYRUSDB_INTERNAL (and assume that this gets logged properly by the caller), or log the incorrect size here and return NOTFOUND. My point is: if calling that with an invalid size is actually an error by the caller, then this function should not hide that just behind a NOTFOUND.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

do you have a preference for the log level?

Copy link
Member

Choose a reason for hiding this comment

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

if it's an error to pass anything else but a validly sized id, then let's log it as ERROR.

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 guess I will have to audit every place where _guid_from_id() is called to see if it tries to validate the emailid.

I added the check just to short-circuit doing a DB lookup, because we know its going to fail.

imap/jmap_mail.c Outdated
return msgid + 1;
static char guidrep[2*MESSAGE_GUID_SIZE+1];

int r = conversations_lookup_jmapid(cstate, emailid + 1, guidrep);
Copy link
Member

Choose a reason for hiding this comment

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

non-actionable comment: I get why this is done here, but it sucks to have a db call introduced into a function that formerly only had the cost of simple pointer arithmetic.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

But that's why it was a function and not just emailid+1 inline right?

Copy link
Member

Choose a reason for hiding this comment

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

Already the emailid+1 was really a code smell and I figured doing that in a single function at least contains that. It's not a big deal to call into conversations now, I guess, at least I don't see this being called in a hot loop (or is it?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It shouldn't be

imap/jmap_mail.c Outdated Show resolved Hide resolved
@ksmurchison ksmurchison force-pushed the 64bit-nanosecond-jmapids branch from 694df67 to b57c6b2 Compare January 27, 2025 13:41
@ksmurchison ksmurchison force-pushed the 64bit-nanosecond-jmapids branch from b57c6b2 to 9f2e29a Compare January 27, 2025 17:46
@ksmurchison ksmurchison force-pushed the 64bit-nanosecond-jmapids branch from 9f2e29a to e46abdc Compare January 27, 2025 22:45
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