Skip to content

release/v2.2007- fix(cleanup): Do not close cache before compaction (#1464)#1488

Merged
jarifibrahim merged 1 commit intorelease/v2.2007from
ibrahim/r2.2007-cache-stop-comapct
Aug 27, 2020
Merged

release/v2.2007- fix(cleanup): Do not close cache before compaction (#1464)#1488
jarifibrahim merged 1 commit intorelease/v2.2007from
ibrahim/r2.2007-cache-stop-comapct

Conversation

@jarifibrahim
Copy link
Contributor

@jarifibrahim jarifibrahim commented Aug 27, 2020

This PR fixes a panic that could happen in case vlog.Open returns an error.
The db.Cleanup would close the cache before compaction would finish and
as a result of that we will end up with send of closed channel panic in ristretto.

(cherry picked from commit edbc380)


This change is Reviewable

This PR fixes a panic that could happen in case `vlog.Open` returns an error.
The `db.Cleanup` would close the cache before compaction would finish and
as a result of that we will end up with `send of closed channel panic` in ristretto.

(cherry picked from commit edbc380)
Copy link
Contributor

@ashish-goswami ashish-goswami left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 1 of 1 files at r1.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jarifibrahim and @manishrjain)


db.go, line 432 at r1 (raw file):

	db.stopMemoryFlush()
	db.stopCompactions()

Please explain the reason here explaining that order of closing should not be changed.

Copy link
Contributor

@ashish-goswami ashish-goswami left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @jarifibrahim and @manishrjain)

@jarifibrahim jarifibrahim merged commit a0d4903 into release/v2.2007 Aug 27, 2020
@jarifibrahim jarifibrahim deleted the ibrahim/r2.2007-cache-stop-comapct branch August 27, 2020 12:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants