-
Notifications
You must be signed in to change notification settings - Fork 25
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
Upgrade FASTER dependency to v2.6.4 #344
Conversation
src/DurableTask.Netherite/StorageLayer/Faster/AzureBlobs/BlobManager.cs
Outdated
Show resolved
Hide resolved
src/DurableTask.Netherite/StorageLayer/Faster/FasterStorageProvider.cs
Outdated
Show resolved
Hide resolved
src/DurableTask.Netherite/StorageLayer/Faster/AzureBlobs/LocalFileCheckpointManager.cs
Outdated
Show resolved
Hide resolved
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.
Overall, looks pretty good. Left some comments.
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.
Looks good, some very minor changes, and waiting for the updated FASTER.
Then we can merge I think, but before releasing, we will still need to run tests to validate (a) users can keep using their existing task hubs without breaking changes, and (b) that this new version of FASTER can run our workloads without new error popping up.
src/DurableTask.Netherite/StorageLayer/Faster/FasterStorageProvider.cs
Outdated
Show resolved
Hide resolved
Starting to run some tests now. |
There appears to be a problem with not being able to load v7.0.0 of Microsoft.Extensions.Logging. I opened a PR to modify FASTER to remain compatible. |
I did a quick validation that it is possible to open old taskhubs after updating. However note that the reverse is not true. Once you have opened a task hub with this version, you cannot go back to an older version! We may need to include a warning? |
@sebastianburckhardt: I don't think we can detect this scenario in Netherite, can we? From looking at the diff, it seems the loading of previous FASTER format is happening at the FASTER level, and our code is oblivious to it, is it not? Assuming this is the case, I'm personally 'ok' with it. However, I think this should be a minor version increase, not a patch version, since we're updating a dependency and the minor version allows us to signal to users that they should pay extra attention to the release notes. |
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.
nit but important: let's please make this a minor version release :) . We're upgrading a dependency and there's implications in terms of backwards compat with previous FASTER storage formats
@bachuv, I think you can merge this once @davidmrdavid approves. We should still run more tests before releasing 1.5.0 but it makes sense to test the entire release, not just this PR.
I have added a PR #368 to update the version to 1.5.0, so it does not need to be part of this PR. |
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.
🚢 it!
Okay, I'll merge it once the CI passes. |
Hmm, those CI failures look somewhat concerning given that FASTER has undergone a lot of updates (and therefore possibly contains new bugs). I will take a look at those failures. |
The CI failures were bugs in FASTER which were fixed in microsoft/FASTER#911. We should be able to move forward as soon as FASTER v2.6.4 is released. |
# Conflicts: # src/DurableTask.Netherite/DurableTask.Netherite.csproj
This PR includes the following changes:
v2.6.1v2.6.2v2.6.3v2.6.4Add validation that customers are not using Netherite V2 with a task hub from Netherite V1 and vice versaRemoved since it's handled in allow reading v4 checkpoints for compatibility FASTER#896Add validation when serializing/deserializing so we can more easily pinpoint data corruption issuesNot needed anymore because we're not revising the storage format now, but we need FASTER to merge allow reading v4 checkpoints for compatibility FASTER#896