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

Work around bug that was breaking q.empty() #1634

Merged
merged 1 commit into from
Apr 5, 2021
Merged

Work around bug that was breaking q.empty() #1634

merged 1 commit into from
Apr 5, 2021

Conversation

relud
Copy link
Contributor

@relud relud commented Apr 5, 2021

peter-wangxu/persist-queue#154 can cause q.empty() to return inaccurate results, which in turn can cause the flush manager to incorrectly think that disks were flushed because this loop wouldn't drain the queue:

while not self.q.empty():

@relud relud requested a review from whd April 5, 2021 23:20
Copy link
Member

@whd whd left a comment

Choose a reason for hiding this comment

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

This does appear to fix the error case we're debugging, but tests are failing and the upstream fix PR is also failing CI.

Copy link
Member

@whd whd left a comment

Choose a reason for hiding this comment

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

I'm slightly worried that we're catching this now instead of in a test case somewhere, but LGTM

@relud
Copy link
Contributor Author

relud commented Apr 5, 2021

I'm slightly worried that we're catching this now instead of in a test case somewhere, but LGTM

filed #1635 to track adding a relevant test

@relud relud merged commit 89659fc into master Apr 5, 2021
@relud relud deleted the resume-queue branch April 5, 2021 23:59
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.

2 participants