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

fix log flush bug #460

Closed
wants to merge 1 commit into from
Closed

Conversation

JacksonYao287
Copy link
Contributor

No description provided.

@@ -422,7 +420,7 @@ void HomeLogStore::flush_sync(logstore_seq_num_t upto_seq_num) {
m_logdev->flush_if_needed(1);

// Step 4: Wait for completion
m_sync_flush_cv.wait(lk, [this, upto_seq_num] { return !m_records.status(upto_seq_num).is_active; });
Copy link
Contributor

Choose a reason for hiding this comment

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

I am not sure if we will have multiple concurrent flush_sync , if we have, this implementation is not right. Although the previous one is also not right:)

Copy link
Contributor

@xiaoxichen xiaoxichen Jun 30, 2024

Choose a reason for hiding this comment

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

Not necessary concurrent...

First call with flush_sync (100), it execute to L416. At the moment, the on_write_completion run to finish and set the m_sync_flush_promise. The flush_sync (100) fulfilled in L417 and not consume the future.
Next time , a flush_sync (1078576) will return immediately by consuming the previous fulfilled future.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

do you mean there is a potential bug? this part is protected by a lock, and everytime we enter this block, m_sync_flush_promise will be updated to a new one.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

you can try it locally in your env. it runs well for hours in mine

@JacksonYao287
Copy link
Contributor Author

will use sync flush , so this pr is unnecessary again

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

Successfully merging this pull request may close these issues.

None yet

3 participants