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

Concurrent merges #118

Merged
merged 24 commits into from
Nov 13, 2019
Merged

Concurrent merges #118

merged 24 commits into from
Nov 13, 2019

Conversation

pascutto
Copy link
Contributor

@pascutto pascutto commented Oct 17, 2019

Fixes #13.
On top of #105 to ease debugging.
This is still very WIP.

@samoht
Copy link
Member

samoht commented Oct 26, 2019

What about adding hooks to ease testing -- maybe a lock that the testing framework can hold to block the merge while doing other operations, to check that there are progress going on.

More generally, I think we should try to redesign the library a bit to make it more easily testable -- I know @craigfe started to work a bit on this on the I/O side, but would be great if we could push this "design-to-ease-tests" at the core of the library, especially for the merges.

@samoht samoht mentioned this pull request Oct 29, 2019
20 tasks
@icristescu
Copy link
Contributor

icristescu commented Oct 29, 2019

Update on debugging:

  • from the logs, it seems that the readonly_and_merge tests fail when a write occurs at the end of a merge. In this case it is expected to fail, a flush is needed between a write and a read of a RO instance. I've updated the tests, and cannot reproduce an error for this particular tests. A test to reproduce this failing behaviour is _write_after_merge.
  • tests readonly_s and readonly still fail and it seems (again from reading the logs) that both fail on the same case: log_async is written during a merge and moved into log at the end of the merge. But between the copy of log_async to log and the Io.sync log the log becomes empty. The merge is concurrent with a find of a RO instance.

@icristescu
Copy link
Contributor

  • tests readonly_s and readonly still fail and it seems (again from reading the logs) that both fail on the same case: log_async is written during a merge and moved into log at the end of the merge. But between the copy of log_async to log and the Io.sync log the log becomes empty. The merge is concurrent with a find of a RO instance.

This is now fixed by the last commit. The issue was a buffer shared between an RO and an RW instance. But there is (at least) one bug remaining.

@icristescu
Copy link
Contributor

But there is (at least) one bug remaining.

This bug was caused by something like* the following scenario:
RW - writes value in log, then calls merge, the value is moved into index, but the generation is not set
RO - calls sync_log and reads generation
RW - clears log, sets generation
RO - reads offset of log but does not see any changes in neither generation nor offset. it does not update its index and therefore cannot find the value.

I proposed a quick fix which is to force an update of the index when the value is not found. This is only to check that the bug is fixed, maybe we can improve it.

*something like this, because is hard to track what really happens as too many logs introduce delays and the bug does not reappear.

@craigfe
Copy link
Member

craigfe commented Oct 30, 2019

After the most-recent commit (e76715c), I have gone through 1000 consecutive test runs without any failures; this seems to have fixed something at least 🎉

@pascutto pascutto marked this pull request as ready for review October 30, 2019 23:01
@craigfe craigfe changed the title [WIP] Concurrent merges Concurrent merges Oct 31, 2019
Copy link
Member

@craigfe craigfe left a comment

Choose a reason for hiding this comment

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

Final round of nit-picks.

@pascutto
Copy link
Contributor Author

Fixed ✔️

@pascutto pascutto merged commit b2c08e5 into mirage:master Nov 13, 2019
samoht added a commit to samoht/tezos that referenced this pull request Nov 14, 2019
See mirage/index#118 for the design discussions
@pascutto pascutto deleted the concurrent-merges branch November 14, 2019 10:51
pascutto pushed a commit that referenced this pull request Nov 14, 2019
Fix an issue during #118 git merge
samoht added a commit to samoht/tezos that referenced this pull request Nov 19, 2019
See mirage/index#118 for the design discussions
samoht added a commit to samoht/tezos that referenced this pull request Nov 20, 2019
See mirage/index#118 for the design discussions
@icristescu icristescu mentioned this pull request Jun 22, 2020
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.

Have non-blocking merges
4 participants