Skip to content

Commit

Permalink
Merge #32465
Browse files Browse the repository at this point in the history 
32465: Revert "engineccl: ignore intents beneath start in MVCCIncrementalIterator" r=benesch a=tbg

This reverts commit 765df40.

Unfortunately, #32421 has turned

> make test TESTS=TestChangefeedSchemaChangeNoBackfill PKG=github.com/cockroachdb/cockroach/pkg/ccl/changefeedccl TESTTIMEOUT=30s TESTFLAGS=-v

and various other tests into failing propositions (they take forever).
This slows down the CDC pipeline and the symptoms affect various other
CDC tests so that skipping the test isn't an attractive option.

Besides, this was clearly not intended, so there's probably something
wrong with the code being reverted here.

We should investigate and submit a fix.

Closes #32461.
Fixes #32433.
Fixes #32444.

Release note: None

Co-authored-by: Tobias Schottdorf <[email protected]>
  • Loading branch information
@tbg
craig[bot] and tbg committed Nov 19, 2018
2 parents 7f6ef6e + ceb4072 commit 5377795
Show file tree
Hide file tree
Showing 2 changed files with 9 additions and 24 deletions.
2 changes: 1 addition & 1 deletion pkg/ccl/storageccl/engineccl/mvcc.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ func (i *MVCCIncrementalIterator) advance() {

metaTimestamp := hlc.Timestamp(i.meta.Timestamp)
if i.meta.Txn != nil {
if i.startTime.Less(metaTimestamp) && !i.endTime.Less(metaTimestamp) {
if !i.endTime.Less(metaTimestamp) {
i.err = &roachpb.WriteIntentError{
Intents: []roachpb.Intent{{
Span: roachpb.Span{Key: i.iter.Key().Key},
Expand Down
31 changes: 8 additions & 23 deletions pkg/ccl/storageccl/engineccl/mvcc_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,7 +61,6 @@ func assertEqualKVs(
expected []engine.MVCCKeyValue,
) func(*testing.T) {
return func(t *testing.T) {
t.Helper()
iter := NewMVCCIncrementalIterator(e, IterOptions{
StartTime: startTime,
EndTime: endTime,
Expand Down Expand Up @@ -181,18 +180,11 @@ func TestMVCCIterateIncremental(t *testing.T) {
if err := engine.MVCCPut(ctx, e, nil, txn2.TxnMeta.Key, txn2.TxnMeta.Timestamp, txn2Val, &txn2); err != nil {
t.Fatal(err)
}
t.Run("intents",
t.Run("intents1",
iterateExpectErr(e, fn, testKey1, testKey1.PrefixEnd(), tsMin, tsMax, "conflicting intents"))
t.Run("intents",
t.Run("intents2",
iterateExpectErr(e, fn, testKey2, testKey2.PrefixEnd(), tsMin, tsMax, "conflicting intents"))
t.Run("intents",
iterateExpectErr(e, fn, keyMin, keyMax, tsMin, ts4, "conflicting intents"))
// Intents above the upper time bound or beneath the lower time bound must
// be ignored (#28358). Note that the lower time bound is exclusive while
// the upper time bound is inclusive.
t.Run("intents", assertEqualKVs(e, fn, keyMin, keyMax, tsMin, ts3, kvs(kv1_3Deleted, kv2_2_2)))
t.Run("intents", assertEqualKVs(e, fn, keyMin, keyMax, ts4, tsMax, kvs()))
t.Run("intents", assertEqualKVs(e, fn, keyMin, keyMax, ts4.Next(), tsMax, kvs()))
t.Run("intents3", assertEqualKVs(e, fn, keyMin, keyMax, tsMin, ts3, kvs(kv1_3Deleted, kv2_2_2)))

intent1 := roachpb.Intent{Span: roachpb.Span{Key: testKey1}, Txn: txn1.TxnMeta, Status: roachpb.COMMITTED}
if err := engine.MVCCResolveWriteIntent(ctx, e, nil, intent1); err != nil {
Expand All @@ -202,7 +194,7 @@ func TestMVCCIterateIncremental(t *testing.T) {
if err := engine.MVCCResolveWriteIntent(ctx, e, nil, intent2); err != nil {
t.Fatal(err)
}
t.Run("intents", assertEqualKVs(e, fn, keyMin, keyMax, tsMin, tsMax, kvs(kv1_4_4, kv2_2_2)))
t.Run("intents4", assertEqualKVs(e, fn, keyMin, keyMax, tsMin, tsMax, kvs(kv1_4_4, kv2_2_2)))
})

t.Run("iterFn=Next", func(t *testing.T) {
Expand Down Expand Up @@ -260,18 +252,11 @@ func TestMVCCIterateIncremental(t *testing.T) {
if err := engine.MVCCPut(ctx, e, nil, txn2.TxnMeta.Key, txn2.TxnMeta.Timestamp, txn2Val, &txn2); err != nil {
t.Fatal(err)
}
t.Run("intents",
t.Run("intents1",
iterateExpectErr(e, fn, testKey1, testKey1.PrefixEnd(), tsMin, tsMax, "conflicting intents"))
t.Run("intents",
t.Run("intents2",
iterateExpectErr(e, fn, testKey2, testKey2.PrefixEnd(), tsMin, tsMax, "conflicting intents"))
t.Run("intents",
iterateExpectErr(e, fn, keyMin, keyMax, tsMin, ts4, "conflicting intents"))
// Intents above the upper time bound or beneath the lower time bound must
// be ignored (#28358). Note that the lower time bound is exclusive while
// the upper time bound is inclusive.
t.Run("intents", assertEqualKVs(e, fn, keyMin, keyMax, tsMin, ts3, kvs(kv1_3Deleted, kv1_2_2, kv1_1_1, kv2_2_2)))
t.Run("intents", assertEqualKVs(e, fn, keyMin, keyMax, ts4, tsMax, kvs()))
t.Run("intents", assertEqualKVs(e, fn, keyMin, keyMax, ts4.Next(), tsMax, kvs()))
t.Run("intents3", assertEqualKVs(e, fn, keyMin, keyMax, tsMin, ts3, kvs(kv1_3Deleted, kv1_2_2, kv1_1_1, kv2_2_2)))

intent1 := roachpb.Intent{Span: roachpb.Span{Key: testKey1}, Txn: txn1.TxnMeta, Status: roachpb.COMMITTED}
if err := engine.MVCCResolveWriteIntent(ctx, e, nil, intent1); err != nil {
Expand All @@ -281,7 +266,7 @@ func TestMVCCIterateIncremental(t *testing.T) {
if err := engine.MVCCResolveWriteIntent(ctx, e, nil, intent2); err != nil {
t.Fatal(err)
}
t.Run("intents", assertEqualKVs(e, fn, keyMin, keyMax, tsMin, tsMax, kvs(kv1_4_4, kv1_3Deleted, kv1_2_2, kv1_1_1, kv2_2_2)))
t.Run("intents4", assertEqualKVs(e, fn, keyMin, keyMax, tsMin, tsMax, kvs(kv1_4_4, kv1_3Deleted, kv1_2_2, kv1_1_1, kv2_2_2)))
})
}

Expand Down

0 comments on commit 5377795

Please sign in to comment.