[HUDI-2559] Millisecond granularity for instant timestamps

[davehagman]

What is the purpose of the pull request

In order to support multiple writers against a single table we need to eliminate (or at least greatly reduce) the chance that multiple writers running in parallel will create instants using the same instant ID (it is currently based on the current time down to second granularity).

The current strategy has a shockingly high rate of instant time collisions even with just two writers. In order to make collisions far less likely this PR adds millisecond granularity to the instant timestamp. It is worth noting that this does not guarantee a collision-free solution so we also include steps to reconcile state in case it does happen.

Brief change log

(for example:)

• Modified HoodieActiveTimeline.COMMIT_FORMATTER to add milliseconds

Verify this pull request

• Ran and verified existing unit tests around timeline operations
• Added unit tests to verify that previous expectations around instant date math still hold true
• Manually tested multiple writers which created hundreds of collision-free timeline entries

Committer checklist

• Has a corresponding JIRA in PR title & commit

• Commit message is descriptive of the change

• CI is green

• Necessary doc changes done or have another open PR

[davehagman]

Still working to test this. We are going to have issues parsing previous commits that do not have the milliseconds on them so I'm coming up with a strategy for that

[davehagman]

@vinothchandar Thanks for the heads up. My last commit abstracts the serde operations into methods on the timeline so that we can properly handle the varying instant formats. I will poke around the compaction code to see if I can spot the special case.

[nsivabalan]

Here are the places that needs fixes

1. compaction trigger based on time elapsed in secs.

• no changes required in metadata table. all we do is string comparison. bootstrap commit time is "0000000000000" (13 chars).
• these are the instants used in bootstrap code. "00000000000000", "00000000000001" and "00000000000002". all of them are 14 chars. again, don't think any changes are required here. string comparison should work out even if there are strings with larger characters.

[davehagman]

@nsivabalan @vinothchandar That should be fine as we now use the methods on the active timeline to parse the instant which will handle second and ms granularity here

[davehagman]

Added test to validate that date math that is being done by compaction still works

[nsivabalan]

Also, checked HoodieHeartBeatClient to check if we have any interplay there.

Looks like this is what we are doing and looks like we may not need any changes. just wanted to bring to others notice.

Every commit will create a file under heartbeat folder. file name is same as instant time.
And at regular intervals will keep overriding it. and we rely on lastModification time of the file to check for expiration at regular cadence.
@davehagman : can you confirm my understanding. old commit times will still be interpreted as is and we don't upgrade them every to new format right. Just that the newer ones we generated are in new format.

[davehagman]

we don't upgrade them every to new format right. Just that the newer ones we generated are in new format.

One potential issue I see is that if you have an old instant without millis such as 20210101120101 and you then parse that to a date (by calling parseDateFromInstantTime), and back to an instant ID you will now have 20210101120101000 (millis attached). If for some reason we try to lookup a specific instant based purely on a Date instance then it will not be found (since the actual persisted instant does not have the 000 appended).

I am trying to find examples of that happening in the codebase but if you know of something then I can focus on that.

[davehagman]

The HoodieTimeline.compareTimestamps method (example usage here) which is used to compare instants, should continue to work fine regardless of second or ms granularity because we are just doing simple string comparisons.

All that to say that old commit times will continue to evaluate against new instant timestamps just fine @nsivabalan

[davehagman]

@nsivabalan These tests are failing even on the master branch for me. Are you able to reproduce this locally? I'm unsure how my changes could have affecting these tests but I'll continue to look into it.

[nsivabalan]

yeah, master is broken ATM. I have put up a patch to fix that. once CI succeeds, will merge the fix in.

[nsivabalan]

@hudi-bot azure run

[nsivabalan]

@hudi-bot azure run

[hudi-bot]

CI report:

• 06ed6bd Azure: FAILURE

Bot commands

@hudi-bot supports the following commands:

• @hudi-bot run azure re-run the last Azure build

[vinothchandar]

we try to lookup a specific instant based purely on a Date instance then it will not be found (since the actual persisted instant does not have the 000 appended).

this is the main part and also all these solo time, bootstrap instant time and the special instants we have in code already, have higher granularity.

[vinothchandar]

Would love having a functional test here with both sec and millisecond granularity mixed into the timeline. I ll check few other things and this should be good to go.

[vinothchandar]

nit: should we use the 17/ instant length, from the constant we have up above?

[nsivabalan]

@vinothchandar : writing a UT in TestHoodieActiveTimeline would suffice, just to compare old and new timestamp formats? or are you looking for tests at write client layer sort of.

[vinothchandar]

I am looking for a FT, to ensure an existing table can transition correctly to the new format. yeah may be writing this at timeline level and ensuring all API guarantees are met is a good addition.

Regardless, we should perform some upgrade testing out of band.

[vinothchandar]

I was trying to understand how this plays with the bootstrap instant times here.

hudi/hudi-common/src/main/java/org/apache/hudi/common/table/timeline/HoodieTimeline.java

Line 89 in e028580

// Instant corresponding to pristine state of the table after its creation

00000000000000 is still YYYYMMDDHHMMSS. So we good.

[vinothchandar]

https://github.com/apache/hudi/blob/master/hudi-common/src/main/java/org/apache/hudi/metadata/HoodieTableMetadata.java#L46 is also same format.

[vinothchandar]

LGTM . thanks @davehagman for this! @nsivabalan can we get more tests in, test it out using yamls as well , ensure things like time based compaction work (may be another UT). and we can land. (rebase and ci)

[nsivabalan]

summary of tests I did with this patch. Looks to be good.

1. Ran test suite yaml for 10 iterations (~50 commits) for both table types with inserts, updates and deletes.
2. Upgrade testing:
   Created a table with 090. After clustering was scheduled killed the deltasreamer. so all commits and replace commit so far are in secs granularity.
   Used this patch and restarted the deltastreamer. new commits were in ms granularity. As expected inflight clustering was rolledback and again attempted with same secs granuality. And succeeds too.
   After few commits, new clustering was seen with ms granularity.

Attaching screen shots from my local testing.

[xushiyan]

closing in favor of #4024