WIP: 64bit nanosecond jmapids

[ksmurchison]

TODO:

• 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]

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

[rsto]

@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]

@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]

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

Thanks, will do.

[rsto]

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.

[rsto]

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?

[ksmurchison]

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?

[rsto]

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.

[ksmurchison]

do you have a preference for the log level?

[rsto]

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

[ksmurchison]

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.

[rsto]

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.

[ksmurchison]

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

[rsto]

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

[ksmurchison]

It shouldn't be