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.

Sign up for GitHub

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

Jump to bottom

DL: remove data from DB on unsubscribe #16786

Merged
merged 21 commits into from
Nov 28, 2023
Merged

DL: remove data from DB on unsubscribe #16786

merged 21 commits into from
Nov 28, 2023

Conversation

fchirica
Copy link
Contributor

@fchirica fchirica commented Nov 7, 2023

Purpose:

Current Behavior:

New Behavior:

Testing Notes:

@fchirica fchirica changed the title Fc.unsubscribe cleanup2 DL: remove data from DB on unsubscribe Nov 7, 2023
@fchirica fchirica added the Fixed Required label for PR that categorizes merge commit message as "Fixed" for changelog label Nov 7, 2023
@fchirica fchirica marked this pull request as ready for review November 8, 2023 00:54
@fchirica fchirica requested a review from a team as a code owner November 8, 2023 00:54
@altendky altendky closed this Nov 16, 2023
@altendky altendky reopened this Nov 16, 2023
altendky
altendky requested changes Nov 16, 2023
View reviewed changes
Copy link
Contributor

@altendky altendky left a comment

Choose a reason for hiding this comment

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

Thanks for implementing this. Nothing stands out as 'broken' except for the dual reader/writer acquisition when deleting. The rest is just cleanup I think. Maybe apply the concepts from the comments on the last test to the other tests as well then I'll look them over in detail as well.

chia/data_layer/data_store.py Outdated Show resolved Hide resolved
chia/data_layer/data_store.py Outdated Show resolved Hide resolved
chia/data_layer/data_store.py Outdated Show resolved Hide resolved
tests/core/data_layer/test_data_store.py Outdated Show resolved Hide resolved
tests/core/data_layer/test_data_store.py Outdated Show resolved Hide resolved
tests/core/data_layer/test_data_store.py Outdated Show resolved Hide resolved
tests/core/data_layer/test_data_store.py Outdated Show resolved Hide resolved
fchirica and others added 3 commits November 17, 2023 01:57
altendky
altendky reviewed Nov 17, 2023
View reviewed changes
tests/core/data_layer/test_data_store.py Outdated Show resolved Hide resolved
chia/data_layer/data_store.py Outdated Show resolved Hide resolved
altendky
altendky requested changes Nov 17, 2023
View reviewed changes
Copy link
Contributor

@altendky altendky left a comment

Choose a reason for hiding this comment

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

Minor minor, then approval. Thanks!

tests/core/data_layer/test_data_store.py Outdated Show resolved Hide resolved
tests/core/data_layer/test_data_store.py Outdated Show resolved Hide resolved
@fchirica fchirica mentioned this pull request Nov 27, 2023
Closed
@emlowe emlowe changed the base branch from main to release/2.1.2 November 27, 2023 18:07
@cmmarslender cmmarslender merged commit 5351980 into release/2.1.2 Nov 28, 2023
254 checks passed
@cmmarslender cmmarslender deleted the fc.unsubscribe_cleanup2 branch November 28, 2023 23:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@altendky altendky altendky approved these changes

@emlowe emlowe emlowe approved these changes

Assignees
No one assigned
Labels
DataLayer Fixed Required label for PR that categorizes merge commit message as "Fixed" for changelog
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@fchirica @altendky @emlowe @cmmarslender