Repository navigation
Add explicit deployment state modes and optional DMS history - #7151
Merged
Merged
Conversation
Collaborator
Integration test reportCommit: 1723931
Top 3 slowest tests (at least 2 minutes):
|
shreyas-goenka
marked this pull request as ready for review
October 23, 2026 12:47
This was referenced Oct 23, 2026
denik
approved these changes
Oct 26, 2026
| // WritingFileWithHistoryEnforced writes state through WAL to resources.json and records DMS history. | ||
| // DMS errors and drain timeouts let the deployment continue. | ||
| // Persists deployment_history_best_effort to reject clients that cannot honor history recording. | ||
| WritingFileWithHistoryEnforced |
Contributor
There was a problem hiding this comment.
// WritingDMS reads and writes resource state through DMS without creating a WAL.
// DMS errors fail the deployment.
WritingDMS
// WritingFileWithHistory writes state through WAL to resources.json and records DMS history.
// DMS errors and drain timeouts let the deployment continue.
WritingFileWithHistory
// WritingFileWithHistoryEnforced writes state through WAL to resources.json and records DMS history.
// DMS errors and drain timeouts let the deployment continue.
// Persists deployment_history_best_effort to reject clients that cannot honor history recording.
WritingFileWithHistoryEnforced
a bit inconsistent, WritingFile and WritingDMS combined results in WritingFileWithHistoryEnforced, "DMS" & "History" both used interchangeably.
Perhaps
- keep WritingDMS
- WritingFileShadowDMSOptional # no feature
- WritingFileShadowDMSRequired # with feature
I also wonder should we drop the last one for now and implement WritingFileShadowDMSOptional end to end?
Contributor
There was a problem hiding this comment.
q: do both WritingFileWithHistory and WritingFileWithHistoryEnforced trigger migration?
Contributor
Author
There was a problem hiding this comment.
I also wonder should we drop the last one for now and implement WritingFileShadowDMSOptional end to end?
The only downside is we make the feature weaker since we can no longer guarentee history. Since I've already implemented this I'd be inclined to keep it, given it's the end state we are moving towards anyways.
Contributor
Author
There was a problem hiding this comment.
WritingFile and WritingDMS combined results in WritingFileWithHistoryEnforced
There's a qualitative difference. WritingDMS reads from DMS and treats it as SOT. WritingFileWithHistoryEnforced does not.
| // | ||
| // DMS reads require a workspace client on ctx. DMS writes and history recording | ||
| // start after a version exists and InitializeOperationBuffer is called. | ||
| func (db *DeploymentState) Open(ctx context.Context, path string, mode Mode, withRecovery WithRecovery, dmsDeployment OpenDmsArgs) error { |
Contributor
There was a problem hiding this comment.
Previously there was only paved path for modes (start with reading then go into writing). Now you can open it in arbitrary mode, that makes it harder to reason. Can we drop mode from here and start with Reading always as before?
Contributor
Author
There was a problem hiding this comment.
By design I did not support a upgrade path for ReadingDMS -> WritingDMS. It just adds complexity (I did try it out). That's why Open takes the argument, to allow us to start at mode WritingDMS.
TBF we could fully cleanup WritingDMS but I'm inclined towards keeping it for now since it will make hte eventual switchover simpler.
We can continue to run local tests with WritingDMS as well.
| ctx = cmdctx.SetWorkspaceClient(ctx, b.WorkspaceClient(ctx)) | ||
| } | ||
| if err := b.DeploymentBundle.StateDB.Open(ctx, localPath, dstate.WithRecovery(false), dstate.WithWrite(false), dstate.WithDeploymentHistory(true), dstate.OpenDmsArgs{DeploymentID: dmsDeploymentID, LastVersionID: lastVersionID}); err != nil { | ||
| if err := b.DeploymentBundle.StateDB.Open(ctx, localPath, dstate.WritingDMS, dstate.WithRecovery(false), dstate.OpenDmsArgs{DeploymentID: dmsDeploymentID, LastVersionID: lastVersionID}); err != nil { |
Contributor
There was a problem hiding this comment.
How did dstate.WithWrite(false) got transformed into dstate.WritingDMS here?
Contributor
Author
There was a problem hiding this comment.
Because dstate.WithWrite(false) is not meaningful for DMS. It would actually require us to add a "upgrade path" for ReadingDMS. I figured the code is simpler to just always start at writing mode. It's a completely independent code path anyways.
Contributor
There was a problem hiding this comment.
then enum value should be called ReadingWritingDMS perhaps?
Collaborator
Integration test reportCommit: 4d88efe
208 interesting tests: 168 MISS, 29 FAIL, 11 flaky
Top 50 slowest tests (at least 2 minutes):
|
Collaborator
Integration test reportCommit: 4d88efe
803 interesting tests: 737 MISS, 66 FAIL
Top 50 slowest tests (at least 2 minutes):
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Changes
This PR adds modes to StateDB setting the stage for recording history in a best effort way. This supported is added via new mode (WritingFileWithHistoryEnforced) that'll be made the default for dpeloyment history in a followup PR.
Existing tests pass - purely refactor.