adapter: Remove legacy builtin item migrations #31218
Conversation
jkosh44
force-pushed
the
remove-legacy-builtin-migration
branch
from
f0b8b24 to
8592941
Compare
|
This feels like a somewhat risky PR for my last week, but it also seems unlikely to get done after and potentially very helpful. It also has the added benefit that the upgrade checks are actually using the same logic that is used in prod. |
jkosh44
force-pushed
the
remove-legacy-builtin-migration
branch
from
8592941 to
480ae88
Compare
jkosh44
changed the title
jkosh44
force-pushed
the
remove-legacy-builtin-migration
branch
3 times, most recently
from
f4b1444 to
4afd695
Compare
Previously, the adapter maintained two different implementations of the builtin item migration framework. One that was used before 0dt and one that was used with 0dt. The 0dt implementation still works when 0dt is turned off, but we wanted to keep the legacy implementation around while the 0dt implementation was still new. The 0dt implementation has been around long enough in production to prove that it works. On the other hand, the legacy implementation has not been tested in production in a long time. This commit removes the legacy implementation and always uses the new implementation. This should reduce the maintenance burden of builtin item migrations. In the future, the builtin item migration framework should be completely removed and replaced with `ALTER TABLE`.
jkosh44
force-pushed
the
remove-legacy-builtin-migration
branch
from
4afd695 to
9465baa
Compare
| /// | ||
| /// Note: These Collections will not be in the Catalog, and are only known about by | ||
| /// the storage controller, which is why we identify them by [`GlobalId`]. | ||
| pub storage_collections_to_drop: BTreeSet<GlobalId>, |
There was a problem hiding this comment.
These are from the times when we had a separate "Stash" (is that the word we had for the things backing the Catalog before we used a persist shard) for storage, so the two could get out of sync and we would have to clean that up?
There was a problem hiding this comment.
This is actually unrelated to the stash. The legacy migration framework worked by dropping all of the migrated items (and dependent items) and creating brand new items with new definitions. The new items would all have new GlobalIds and be treated as brand new items.
The new migration framework doesn't drop any items, but it swaps out the shards of existing items. So we do finalize shards, but we don't drop any items.
| @@ -679,7 +558,7 @@ impl Catalog { | |||
| let mut txn = storage.transaction().await?; | |||
|
|
|||
| storage_collections | |||
| .initialize_state(&mut txn, collections, storage_collections_to_drop) | |||
| .initialize_state(&mut txn, collections) | |||
There was a problem hiding this comment.
Interesting! We really never drop storage collections as part of migrations?
There was a problem hiding this comment.
Interesting! We really never drop storage collections as part of migrations?
Yep no storage collections are dropped, but we do finalize shards and create new ones.
| builtin_item_migration_config: BuiltinItemMigrationConfig { | ||
| // We don't actually want to write anything down, so use an in-memory persist | ||
| // client. | ||
| persist_client: PersistClient::new_for_tests().await, |
There was a problem hiding this comment.
This is the scariest part of the PR IMO since it's hard (impossible?) to test the upgrade checks in the cloud repo.
Previously, the adapter maintained two different implementations of the
builtin item migration framework. One that was used before 0dt and one
that was used with 0dt. The 0dt implementation still works when 0dt is
turned off, but we wanted to keep the legacy implementation around
while the 0dt implementation was still new. The 0dt implementation has
been around long enough in production to prove that it works. On the
other hand, the legacy implementation has not been tested in production
in a long time.
This commit removes the legacy implementation and always uses the new
implementation. This should reduce the maintenance burden of builtin
item migrations.
In the future, the builtin item migration framework should be
completely removed and replaced with
ALTER TABLE.Motivation
This PR refactors existing code.
Checklist
$T ⇔ Proto$Tmapping (possibly in a backwards-incompatible way), then it is tagged with aT-protolabel.