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

Perform "quick commit" in materializer service #280

Merged
merged 7 commits into from
May 24, 2023

Conversation

sandreae
Copy link
Member

@sandreae sandreae commented Feb 26, 2023

Uses p2panda/p2panda#485 to perform a quick commit on a document instead of materializing from scratch.

📋 Checklist

  • Add tests that cover your changes
  • Add this PR to the Unreleased section in CHANGELOG.md
  • Link this PR to any issues it closes
  • New files contain a SPDX license header

@codecov
Copy link

codecov bot commented Feb 26, 2023

Codecov Report

Patch coverage: 95.18% and project coverage change: +0.14 🎉

Comparison is base (8cd21bd) 92.46% compared to head (0cee8e2) 92.61%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #280      +/-   ##
==========================================
+ Coverage   92.46%   92.61%   +0.14%     
==========================================
  Files          70       70              
  Lines        6573     6652      +79     
==========================================
+ Hits         6078     6161      +83     
+ Misses        495      491       -4     
Impacted Files Coverage Δ
aquadoggo/src/materializer/service.rs 95.27% <95.12%> (-0.14%) ⬇️
aquadoggo/src/db/stores/document.rs 91.63% <100.00%> (ø)

... and 3 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@sandreae
Copy link
Member Author

This PR is still open, and I'm actually nervous to merge it, cos I think it will hide any concurrency bugs even deeper 😅

@adzialocha
Copy link
Member

This PR is still open, and I'm actually nervous to merge it, cos I think it will hide any concurrency bugs even deeper 😅

Oooh interesting, did you observe new / more issues on that branch?

@sandreae
Copy link
Member Author

This PR is still open, and I'm actually nervous to merge it, cos I think it will hide any concurrency bugs even deeper sweat_smile

Oooh interesting, did you observe new / more issues on that branch?

No new observations I'm afraid. Just the thought that by using "quick commits" to update documents without issuing reduce tasks, then we'd probably make reproducing any concurrency bugs even hard....

@adzialocha
Copy link
Member

I think the errors I get are all happening before materialization (it's always about duplicate seq num, log id or some missing operation id hashes).

Let me run this branch a couple of times and see if I can tell a difference in the issues. Would be nice to merge this!

@adzialocha
Copy link
Member

I think the errors I get are all happening before materialization (it's always about duplicate seq num, log id or some missing operation id hashes).

Let me run this branch a couple of times and see if I can tell a difference in the issues. Would be nice to merge this!

Same bugs as in #270, so all good 😆

@sandreae
Copy link
Member Author

I think the errors I get are all happening before materialization (it's always about duplicate seq num, log id or some missing operation id hashes).
Let me run this branch a couple of times and see if I can tell a difference in the issues. Would be nice to merge this!

Same bugs as in #270, so all good laughing

Well, that's good..... lolz...

@sandreae
Copy link
Member Author

I'll get this ready for merging when I have time then 👍

@sandreae sandreae force-pushed the incremental-document-update branch from 1107400 to 1a9d16d Compare May 23, 2023 22:34
@sandreae sandreae marked this pull request as ready for review May 23, 2023 23:27
@adzialocha adzialocha self-requested a review May 24, 2023 07:21
@adzialocha adzialocha merged commit a5b3d80 into main May 24, 2023
9 checks passed
@adzialocha adzialocha deleted the incremental-document-update branch May 24, 2023 07:22
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.

None yet

2 participants