Skip to content

Commit

Permalink
Revert "engineccl: ignore intents beneath start in MVCCIncrementalIte…
Browse files Browse the repository at this point in the history 
…rator"

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
  • Loading branch information
@tbg
tbg committed Nov 19, 2018
1 parent d52576a commit ceb4072
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 ceb4072

Please sign in to comment.