Repository navigation
Add explicit deployment state modes and optional DMS history - #7151
Conversation
Integration test reportCommit: 1723931
Top 3 slowest tests (at least 2 minutes):
|
| // 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 |
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?
There was a problem hiding this comment.
q: do both WritingFileWithHistory and WritingFileWithHistoryEnforced trigger migration?
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.
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 { |
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?
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 { |
There was a problem hiding this comment.
How did dstate.WithWrite(false) got transformed into dstate.WritingDMS here?
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.
There was a problem hiding this comment.
then enum value should be called ReadingWritingDMS perhaps?
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.