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

[cmds][scd/store] Add 'db-manager scd-evict' subcommand enabling listing and deletion of expired SCD entities #1116

Merged
merged 4 commits into from
Oct 15, 2024

Conversation

mickmis
Copy link
Contributor

@mickmis mickmis commented Sep 12, 2024

Addresses #1074

This PR adds a CLI tool db-manager scd-evict that enables deletion of expired entities. See enclosed README for more details on its usage.

Do note the approach taken when entities do not have end times: the last update times are used.

@mickmis mickmis force-pushed the 1074/db-evictor branch 3 times, most recently from 6390f18 to f33d832 Compare September 13, 2024 13:41
@mickmis mickmis changed the title [cmds][scd/store] Add 'db-evictor' command enabling cleanup of expired op intents and subscriptions [cmds][scd/store] Add 'db-evictor' command enabling deletion of expired op intents and subscriptions Sep 13, 2024
@mickmis mickmis marked this pull request as ready for review September 13, 2024 13:59
Copy link
Contributor

@barroco barroco left a comment

Choose a reason for hiding this comment

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

You will find a first round of comments. In addition, I think we could leverage the infrastructure used for RID to automate the tests you have made manually without too much effort: https://github.com/interuss/dss/blob/master/Makefile#L113.

Dockerfile Outdated Show resolved Hide resolved
cmds/db-evictor/main.go Outdated Show resolved Hide resolved
cmds/db-evictor/main.go Outdated Show resolved Hide resolved
pkg/scd/store/cockroach/operational_intents.go Outdated Show resolved Hide resolved
@mickmis mickmis requested a review from barroco September 24, 2024 11:25
@mickmis mickmis changed the title [cmds][scd/store] Add 'db-evictor' command enabling deletion of expired op intents and subscriptions [cmds][scd/store] Add 'db-manager scd-evict' subcommand enabling listing and deletion of expired SCD entities Sep 24, 2024
@mickmis mickmis force-pushed the 1074/db-evictor branch 3 times, most recently from 973e677 to f3a6bfa Compare September 24, 2024 12:12
@mickmis mickmis marked this pull request as draft September 24, 2024 15:05
@mickmis
Copy link
Contributor Author

mickmis commented Sep 24, 2024

FYI still need to test manually the CLI + add unit tests.

@callumdmay
Copy link

Hey @mickmis where are we currently at with this tool? We just updated to v0.18 but we've accumulated a significant number of dangling subscriptions over the last month and they are having a significant affect on latency so we're hoping to leverage this tool ASAP

@barroco
Copy link
Contributor

barroco commented Oct 11, 2024

Hi @callumdmay, thank you for your patience. @mickmis is ooo today, he will get back to you on Monday and should be able to progress on this with top priority.

@callumdmay
Copy link

Great to hear and thanks for all the work here!

@mickmis
Copy link
Contributor Author

mickmis commented Oct 14, 2024

Hi @callumdmay, sorry I've had to slow down significantly my work on this issue for the last few weeks. I've actually resumed that last Thursday and I expect this PR to be merged by tomorrow.

@mickmis mickmis marked this pull request as ready for review October 14, 2024 10:00
@mickmis
Copy link
Contributor Author

mickmis commented Oct 14, 2024

@barroco this is now ready for another review. I've added unit tests and tested the tool manually in addition.

Copy link
Contributor

@barroco barroco left a comment

Choose a reason for hiding this comment

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

Looks good to me as a first version. Please take a note on the performance concern.

cmds/db-manager/cleanup/README.md Outdated Show resolved Hide resolved
cmds/db-manager/cleanup/scd-evict.go Outdated Show resolved Hide resolved
cmds/db-manager/cleanup/scd-evict.go Outdated Show resolved Hide resolved
cmds/db-manager/cleanup/scd-evict.go Outdated Show resolved Hide resolved
@mickmis mickmis merged commit 763ffa1 into interuss:master Oct 15, 2024
6 checks passed
@mickmis mickmis deleted the 1074/db-evictor branch October 15, 2024 09:15
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.

3 participants