fix: Various Control Plane migration fixes #552
Merged
Conversation
This file contains 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
morgsmccauley
force-pushed
the
fix/control-plane-migration
branch
2 times, most recently
from
ca95d01
to
b7f3562
Compare
morgsmccauley
commented
Feb 6, 2024
| @@ -34,16 +34,17 @@ impl RedisClientImpl { | |||
| }) | |||
| } | |||
|
|
|||
| pub async fn get<T, U>(&self, key: T) -> anyhow::Result<U> | |||
| pub async fn get<T, U>(&self, key: T) -> anyhow::Result<Option<U>> | |||
There was a problem hiding this comment.
Not actually directly related, but quite a nice change to have
morgsmccauley
commented
Feb 6, 2024
| #[derive(serde::Deserialize, serde::Serialize, Debug)] | ||
| struct DenylistEntry { | ||
| account_id: AccountId, | ||
| v1_ack: bool, | ||
| migrated: bool, |
There was a problem hiding this comment.
This can still be deserialized without these fields, but it means they will be removed when serializing again.
morgsmccauley
commented
Feb 6, 2024
| @@ -104,8 +113,7 @@ async fn migrate_account( | |||
| .context("Failed to merge streams")?; | |||
| } | |||
|
|
|||
| // TODO Uncomment when V2 correctly continues from V1 stop point | |||
| // set_migrated_flag(redis_client, account_id)?; | |||
| set_migrated_flag(redis_client, account_id)?; | |||
There was a problem hiding this comment.
We need to set the migrated flag to prevent further attempts to migrate
morgsmccauley
commented
Feb 6, 2024
| @@ -14,12 +14,17 @@ pub struct AllowlistEntry { | |||
| v1_ack: bool, | |||
| migrated: bool, | |||
| failed: bool, | |||
| v2_control: bool, | |||
There was a problem hiding this comment.
Since we have to set the migrated flag, this allows us to still prevent V2 from taking control. Providing time to sanity check the migration itself.
morgsmccauley
force-pushed
the
fix/control-plane-migration
branch
from
b7f3562
to
0ddbb6e
Compare
morgsmccauley
changed the title
|
Going to merge these to unblock testing - feel free to review in retrospect |
Merged
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.
No description provided.