Skip to content
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

WIP: Universe root deletion RPC #335

Merged
merged 5 commits into from
Jun 20, 2023
Merged

WIP: Universe root deletion RPC #335

merged 5 commits into from
Jun 20, 2023

Conversation

jharveyb
Copy link
Collaborator

Interim fix for #320, which should be reworked once we land other DB changes.

Add a command to delete specific universe trees, which is useful for avoiding universe sync failures. Ideally this would be done more automatically with cascading deletes in the DB.

Successfully tested as fixing the root cause of #320, my tapd no longer shows an incorrect root for that asset group. Sync with the default universe still fails.

tapdb/mssmt.go Show resolved Hide resolved
tapdb/universe.go Show resolved Hide resolved
tapdb/sqlc/queries/mssmt.sql Show resolved Hide resolved
tapdb/universe.go Show resolved Hide resolved
tapdb/universe.go Outdated Show resolved Hide resolved
@Roasbeef
Copy link
Member

Roasbeef commented Jun 1, 2023

Missing an entry for REST in the yaml file also.

@jharveyb
Copy link
Collaborator Author

jharveyb commented Jun 1, 2023

Addressed review feedback + tested locally with this mint script (on regtest, not testnet) https://github.com/jharveyb/remintooor/blob/main/mint_assets.sh

After minting, I synced the universe tree of 100 assets to a second tapd, deleted the universe root on that second tapd, and re-synced the same universe root without issue. Root deletion on the minting node also succeeded.

@Roasbeef
Copy link
Member

Roasbeef commented Jun 1, 2023

Diff looking better, I think this is just missing unit tests + an itest (to replicate the scenario we execute live on the instance).

Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM pending some unit and integration tests!

mssmt/store.go Outdated Show resolved Hide resolved
taprpc/universerpc/universe.proto Outdated Show resolved Hide resolved
@jharveyb jharveyb force-pushed the universe_root_deletion branch 2 times, most recently from 836e041 to 7cabe13 Compare June 16, 2023 07:15
@jharveyb
Copy link
Collaborator Author

Added unit tests + itests, no large changes otherwise.

Needed one change here to handle trees with missing leaves:

if leaf, ok := d.leaves[key]; ok {

@jharveyb jharveyb marked this pull request as ready for review June 16, 2023 07:18
Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found one potential issue, the rest looks pretty good!

mssmt/store.go Outdated Show resolved Hide resolved
mssmt/store.go Outdated Show resolved Hide resolved
cmd/tapcli/universe.go Show resolved Hide resolved
Copy link
Member

@guggero guggero left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🎉

@jharveyb jharveyb requested a review from ffranr June 16, 2023 17:24
Copy link
Contributor

@ffranr ffranr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great improvements 👍

One or two nits, and one doc fix.

tapdb/mssmt_test.go Show resolved Hide resolved
tapdb/universe.go Show resolved Hide resolved
tapdb/universe.go Outdated Show resolved Hide resolved
Comment on lines +626 to +630
// Instantiate a compact tree so we can delete the MS-SMT
// backing the universe.
universeTree := mssmt.NewCompactedTree(
newTreeStoreWrapperTx(db, b.smtNamespace),
)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not proposing a further change in this PR, but I think our language is sub-optimal here.

We're not actually creating a new compact tree or instantiating a compact tree. We're creating a database handle for a compact tree. I think we should rename NewCompactedTree to NewCompactedTreeHandle or something like that.

@@ -354,6 +371,29 @@ func TestUniverseTreeIsolation(t *testing.T) {
}
return false
}))

// We should be able to delete one Universe with no effect on the other.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

tapdb/universe_test.go Outdated Show resolved Hide resolved
cmd/tapcli/universe.go Outdated Show resolved Hide resolved
@guggero guggero enabled auto-merge June 20, 2023 15:17
@guggero guggero added this pull request to the merge queue Jun 20, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Jun 20, 2023
@guggero guggero added this pull request to the merge queue Jun 20, 2023
@jharveyb
Copy link
Collaborator Author

CI failure in postgres itests, but the logs show all itests passing, in ~520s; not sure what the issue may be there?

Merged via the queue into main with commit 1f8dd8a Jun 20, 2023
@guggero guggero deleted the universe_root_deletion branch June 20, 2023 15:58
@guggero
Copy link
Member

guggero commented Jun 20, 2023

Yeah, saw that too, probably just a weird timing issue in the itest framework.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants