cli: TestDumpData fails with KV error #48147
Comments
|
Tried repro'ing this via https://github.com/cockroachdb/cockroach/compare/master...tbg:TestDumpData-48147?expand=1 (to avoid stressrace) but no dice so far (20 8cpu nodes):
Going to try stressrace now. Looking at the error above, it seems that we're evaluating a point lookup (i.e. One thing we should certainly do is dump a stack trace so it's more obvious where it's happening, I'll send a PR. |
It's going to be much easier figuring out what happened when the stack is included. See cockroachdb#48147. Release note: None
48161: spanset: print stack with assertion error r=petermattis,nvanbenschoten a=tbg It's going to be much easier figuring out what happened when the stack is included. See #48147. Release note: None Co-authored-by: Tobias Schottdorf <[email protected]>
Going to put this on the back burner. |
|
So maybe I'm wrong and this is due to my PR as I have another failure on CI: https://teamcity.cockroachdb.com/viewLog.html?buildId=1909094&buildTypeId=Cockroach_UnitTests. Relevant portion of the error (with your stacktrace): Let me see if I can reproduce locally. |
|
Yep, reproduces easily on #48145 using |
|
And fails reliably on master with |
|
I'm going to tag @itsbilal into this investigation as it looks like it could be a bug in |
|
The span assertions are off under non race, so this is expected.
…On Thu, Apr 30, 2020, 01:22 Peter Mattis ***@***.***> wrote:
I'm going to tag @itsbilal <https://github.com/itsbilal> into this
investigation as it looks like it could be a bug in pebbleMVCCScanner.
Somewhat strange that this happens reliably (i.e. every run) under
testrace, but not at all under non-race (i.e. stress).
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
<#48147 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ABGXPZGSG4GJXU6JYDDO2LTRPCZCNANCNFSM4MTVWHGA>
.
|
|
This one is pretty easy to explain, and I confirmed it with some good ol' debug statements. It's not concerning at all. When we RocksDB is also |
|
Quick work tracking this down. Any thoughts on what should be done to fix it? Should we change |
|
Given an additional seek is expensive anyway, it's probably best to avoid it in We're usually wary of making changes to |
SGTM |
Previously, when doing an MVCCGet with the Pebble MVCC scanner, we would advance keys after getting (or not getting) the value we're looking for. In some cases this could result in a seek, such as if there are too many revisions of that MVCC key. Seeks are costly and at best avoided when it's ultimatey unnecessary. Another side effect of doing this seek was that it would trip an assertion in spanSet.Iterator that checked if all reads were happening on allowed key spans. This caused TestDumpData to fail on Pebble (but not on RocksDB since the RocksDB MVCC scanner is specialized in C++ and doesn't flow through the same assertion for seeks). There's one additional change, stemming from a follow-up conversation in cockroachdb#48160, around unifying logic for terminating an MVCC scan. The getFromIntentHistory() case also calls into addAndAdvance() now. Fixes cockroachdb#48147. Release note: None.
48239: storage: Don't advance keys in MVCCGet with Pebble r=itsbilal a=itsbilal Previously, when doing an MVCCGet with the Pebble MVCC scanner, we would advance keys after getting (or not getting) the value we're looking for. In some cases this could result in a seek, such as if there are too many revisions of that MVCC key. Seeks are costly and at best avoided when it's ultimatey unnecessary. Another side effect of doing this seek was that it would trip an assertion in spanSet.Iterator that checked if all reads were happening on allowed key spans. This caused TestDumpData to fail on Pebble (but not on RocksDB since the RocksDB MVCC scanner is specialized in C++ and doesn't flow through the same assertion for seeks). There's one additional change, stemming from a follow-up conversation in #48160, around unifying logic for terminating an MVCC scan. The getFromIntentHistory() case also calls into addAndAdvance() now. Fixes #48147. Release note: None. Co-authored-by: Bilal Akhtar <[email protected]>
Failure seen on this CI build: https://teamcity.cockroachdb.com/viewLog.html?buildId=1908081&buildTypeId=Cockroach_UnitTests
I'm pretty sure this failure is unrelated to that PR. I didn't look closely at the failure. One interesting bit is this error which I believe indicates a KV programming error:
Cc @tbg for triage
The text was updated successfully, but these errors were encountered: