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

WIP (do not merge): fix(rollups): Write rolled-up keys at ts+1 (#7957) (#7959) #8460

Closed
wants to merge 4 commits into from

Conversation

joshua-goldstein
Copy link
Contributor

@joshua-goldstein joshua-goldstein commented Nov 28, 2022

From original commit:

Write rolled up keys at (max ts of the deltas + 1) because if we write the rolled-up keys at the same ts as that of the delta, then in case of WAL replay the rolled-up key would get over-written by the delta which can bring DB to an invalid state.

(cherry picked from commit 3831b49)

Cherry pick from #7959. Curiously, CI-dgraph and CI-dgraph-load tests almost consistently fail on this change. See the checks for more info. It would be good to understand why checks are usually failing, and also to understand why they on occasion do pass.

Write rolled up keys at (max ts of the deltas + 1) because if we write
the rolled-up keys at the same ts as that of the delta, then in case of
WAL replay the rolled-up key would get over-written by the delta which
can bring DB to an invalid state.

(cherry picked from commit 3831b49)
@CLAassistant
Copy link

CLAassistant commented Nov 28, 2022

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ joshua-goldstein
❌ ahsanbarkati
You have signed the CLA already but the status is still pending? Let us recheck it.

@joshua-goldstein joshua-goldstein self-assigned this Nov 28, 2022
@joshua-goldstein joshua-goldstein added the cherry-pick-22.0.2 Label for all cherry-picks for 22.0.2 Release label Nov 28, 2022
@joshua-goldstein joshua-goldstein marked this pull request as ready for review November 28, 2022 21:39
@coveralls
Copy link

coveralls commented Nov 28, 2022

Coverage Status

Coverage increased (+0.01%) to 37.201% when pulling b79ad90 on cherry-pick-7959 into 9a2427e on main.

@joshua-goldstein joshua-goldstein changed the title fix(rollups): Write rolled-up keys at ts+1 (#7957) (#7959) WIP (DO NOT MERGE): fix(rollups): Write rolled-up keys at ts+1 (#7957) (#7959) Nov 29, 2022
@joshua-goldstein joshua-goldstein changed the title WIP (DO NOT MERGE): fix(rollups): Write rolled-up keys at ts+1 (#7957) (#7959) WIP (do not merge): fix(rollups): Write rolled-up keys at ts+1 (#7957) (#7959) Nov 29, 2022
@joshua-goldstein joshua-goldstein marked this pull request as draft November 29, 2022 18:14
@joshua-goldstein
Copy link
Contributor Author

With this change we see almost consistent failures in dgraph-load-tests and 50/50 passes and failures in dgraph-tests (see checks on the PR). This requires further investigation and should not be merged for now. Since this change was introduced after roaring bitmaps were introduced, it is possible there are missing side effects from that switch.

@mangalaman93
Copy link
Contributor

yeah, I do not follow the reasoning either well. It could be a problem to use a timestamp ts+1 too with the same logic.

@matthewmcneely
Copy link
Contributor

Good to see that the comprehensive CI test changes that @skrdgraph and the rest of the team implemented enabled us to see the issue in aggregate. It paid for itself here...

@MichelDiz MichelDiz added the dgraph Issue or PR created by an internal Dgraph contributor. label Mar 7, 2023
@joshua-goldstein joshua-goldstein marked this pull request as ready for review March 9, 2023 18:02
@joshua-goldstein
Copy link
Contributor Author

Closing this as a fix will eventually be merged in #8759

@mangalaman93 mangalaman93 deleted the cherry-pick-7959 branch April 18, 2023 20:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cherry-pick-22.0.2 Label for all cherry-picks for 22.0.2 Release dgraph Issue or PR created by an internal Dgraph contributor.
Development

Successfully merging this pull request may close these issues.

7 participants