Skip to content

Commit cef67ea

Browse files
BenHuddlestondaverigby
BenHuddleston
authored and
daverigby
committed
MB-45949: Overwrite logically deleted items in delWithMeta
In VBucket::deleteWithMeta we skip deleting any item which is logically deleted. If a collection is resurrected though then this will prevent us from replicating a tombstone (belonging to the new generation of the collection). Change-Id: I4f7600307929e9b1e5cf143b40db0b14daf9cf84 Reviewed-on: http://review.couchbase.org/c/kv_engine/+/152223 Reviewed-by: Jim Walker <[email protected]> Tested-by: Build Bot <[email protected]>
1 parent da3795c commit cef67ea

File tree

4 files changed

+180
-16
lines changed

4 files changed

+180
-16
lines changed

‎engines/ep/src/vbucket.cc‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2246,7 +2246,8 @@ cb::engine_errc VBucket::deleteWithMeta(
22462246
auto& hbl = htRes.pending.getHBL();
22472247

22482248
if (v && cHandle.isLogicallyDeleted(v->getBySeqno())) {
2249-
return cb::engine_errc::no_such_key;
2249+
// v is not really here, operate like it's not and skip conflict checks
2250+
checkConflicts = CheckConflicts::No;
22502251
}
22512252

22522253
// Need conflict resolution?

‎engines/ep/tests/module_tests/collections/collections_dcp_producers.cc‎

Lines changed: 68 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -202,6 +202,74 @@ cb::engine_errc CollectionsDcpTestProducers::mutation(
202202
return ret;
203203
}
204204

205+
cb::engine_errc CollectionsDcpTestProducers::deletion(
206+
uint32_t opaque,
207+
cb::unique_item_ptr itm,
208+
Vbid vbucket,
209+
uint64_t by_seqno,
210+
uint64_t rev_seqno,
211+
cb::mcbp::DcpStreamId sid) {
212+
auto ret = cb::engine_errc::success;
213+
if (consumer) {
214+
auto& item = *static_cast<Item*>(itm.get());
215+
216+
ret = consumer->deletion(
217+
opaque,
218+
item.getKey(),
219+
{reinterpret_cast<const uint8_t*>(item.getData()),
220+
item.getNBytes()},
221+
0,
222+
item.getDataType(),
223+
item.getCas(),
224+
replicaVB,
225+
by_seqno,
226+
rev_seqno,
227+
{});
228+
}
229+
230+
MockDcpMessageProducers::deletion(
231+
opaque, std::move(itm), vbucket, by_seqno, rev_seqno, sid);
232+
233+
return ret;
234+
}
235+
236+
cb::engine_errc CollectionsDcpTestProducers::deletion_v2(
237+
uint32_t opaque,
238+
cb::unique_item_ptr itm,
239+
Vbid vbucket,
240+
uint64_t by_seqno,
241+
uint64_t rev_seqno,
242+
uint32_t delete_time,
243+
cb::mcbp::DcpStreamId sid) {
244+
auto ret = cb::engine_errc::success;
245+
if (consumer) {
246+
auto& item = *static_cast<Item*>(itm.get());
247+
248+
ret = consumer->deletionV2(
249+
opaque,
250+
item.getKey(),
251+
{reinterpret_cast<const uint8_t*>(item.getData()),
252+
item.getNBytes()},
253+
0,
254+
item.getDataType(),
255+
item.getCas(),
256+
replicaVB,
257+
by_seqno,
258+
rev_seqno,
259+
delete_time);
260+
}
261+
262+
MockDcpMessageProducers::deletion_v2(opaque,
263+
std::move(itm),
264+
vbucket,
265+
by_seqno,
266+
rev_seqno,
267+
delete_time,
268+
sid);
269+
270+
return ret;
271+
}
272+
205273
cb::engine_errc CollectionsDcpTestProducers::prepare(
206274
uint32_t opaque,
207275
cb::unique_item_ptr itm,

‎engines/ep/tests/module_tests/collections/collections_dcp_producers.h‎

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,6 +47,21 @@ class CollectionsDcpTestProducers : public MockDcpMessageProducers {
4747
uint8_t nru,
4848
cb::mcbp::DcpStreamId sid) override;
4949

50+
cb::engine_errc deletion(uint32_t opaque,
51+
cb::unique_item_ptr itm,
52+
Vbid vbucket,
53+
uint64_t by_seqno,
54+
uint64_t rev_seqno,
55+
cb::mcbp::DcpStreamId sid) override;
56+
57+
cb::engine_errc deletion_v2(uint32_t opaque,
58+
cb::unique_item_ptr itm,
59+
Vbid vbucket,
60+
uint64_t by_seqno,
61+
uint64_t rev_seqno,
62+
uint32_t delete_time,
63+
cb::mcbp::DcpStreamId sid) override;
64+
5065
cb::engine_errc prepare(uint32_t opaque,
5166
cb::unique_item_ptr itm,
5267
Vbid vbucket,

‎engines/ep/tests/module_tests/collections/evp_store_collections_dcp_test.cc‎

Lines changed: 95 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -2644,7 +2644,7 @@ TEST_P(CollectionsDcpParameterizedTest, seqno_advanced_from_disk_to_memory) {
26442644

26452645
class CollectionsDcpPersistentOnly : public CollectionsDcpParameterizedTest {
26462646
public:
2647-
void resurrectionTest(bool dropAtEnd);
2647+
void resurrectionTest(bool dropAtEnd, bool updateItemPath, bool deleteItem);
26482648
void resurrectionStatsTest(bool reproduceUnderflow);
26492649
};
26502650

@@ -2655,7 +2655,9 @@ class CollectionsDcpPersistentOnly : public CollectionsDcpParameterizedTest {
26552655
// de-duped the drop{c1} away so the meta-data became 'corrupt'.
26562656
// This test re-creates the events which lead to a underflow in stats and
26572657
// metadata corruption that lead to warmup failing.
2658-
void CollectionsDcpPersistentOnly::resurrectionTest(bool dropAtEnd) {
2658+
void CollectionsDcpPersistentOnly::resurrectionTest(bool dropAtEnd,
2659+
bool updateItemPath,
2660+
bool deleteItem) {
26592661
VBucketPtr vb = store->getVBucket(vbid);
26602662

26612663
// Add the fruit collection
@@ -2698,7 +2700,21 @@ void CollectionsDcpPersistentOnly::resurrectionTest(bool dropAtEnd) {
26982700

26992701
// Store one key in the last generation of the target collection, this
27002702
// allows stats to be tested at the end.
2701-
store_item(vbid, makeStoredDocKey("pear", target), "shaped");
2703+
StoredDocKey lastGenKey;
2704+
if (updateItemPath) {
2705+
lastGenKey = makeStoredDocKey("orange", target);
2706+
} else {
2707+
lastGenKey = makeStoredDocKey("pear", target);
2708+
}
2709+
2710+
store_item(vbid,
2711+
lastGenKey,
2712+
"shaped",
2713+
0 /*exptime*/,
2714+
{cb::engine_errc::success},
2715+
PROTOCOL_BINARY_DATATYPE_JSON,
2716+
{},
2717+
deleteItem);
27022718

27032719
if (dropAtEnd) {
27042720
cm.remove(target);
@@ -2733,8 +2749,12 @@ void CollectionsDcpPersistentOnly::resurrectionTest(bool dropAtEnd) {
27332749
stepDelete();
27342750
stepCreate();
27352751

2736-
stepAndExpect(cb::mcbp::ClientOpcode::DcpMutation,
2737-
cb::engine_errc::success);
2752+
auto messageType = cb::mcbp::ClientOpcode::DcpMutation;
2753+
if (deleteItem) {
2754+
messageType = cb::mcbp::ClientOpcode::DcpDeletion;
2755+
}
2756+
2757+
stepAndExpect(messageType, cb::engine_errc::success);
27382758

27392759
if (dropAtEnd) {
27402760
stepDelete();
@@ -2755,12 +2775,20 @@ void CollectionsDcpPersistentOnly::resurrectionTest(bool dropAtEnd) {
27552775

27562776
GetValue gv = store->get(key, vbid, cookie, options);
27572777

2778+
if (gv.getStatus() == cb::engine_errc::would_block) {
2779+
runBGFetcherTask();
2780+
gv = store->get(key, vbid, cookie, options);
2781+
}
2782+
2783+
auto expectedGetStatus = cb::engine_errc::no_such_key;
27582784
if (dropAtEnd) {
2759-
EXPECT_EQ(cb::engine_errc::unknown_collection, gv.getStatus());
2760-
} else {
2761-
EXPECT_EQ(cb::engine_errc::no_such_key, gv.getStatus());
2785+
expectedGetStatus = cb::engine_errc::unknown_collection;
2786+
} else if (updateItemPath && !deleteItem) {
2787+
expectedGetStatus = cb::engine_errc::success;
27622788
}
27632789

2790+
EXPECT_EQ(expectedGetStatus, gv.getStatus());
2791+
27642792
VBucketPtr rvb = store->getVBucket(replicaVB);
27652793
EXPECT_EQ(rvb->getHighSeqno(), vb->getHighSeqno());
27662794
// Now read back the persisted manifest(s) and validate that the fruit
@@ -2822,9 +2850,25 @@ void CollectionsDcpPersistentOnly::resurrectionTest(bool dropAtEnd) {
28222850
checkDropped(rvb->getShard()->getRWUnderlying()->getDroppedCollections(
28232851
replicaVB));
28242852

2853+
// Check value in VB before erasure
2854+
auto expected = 2;
2855+
if (updateItemPath) {
2856+
expected = 1;
2857+
}
2858+
2859+
if (deleteItem) {
2860+
expected--;
2861+
}
2862+
2863+
ASSERT_EQ(expected, vb->getNumItems());
28252864
runEraser();
28262865

2827-
auto checkKVS = [dropAtEnd, &target](KVStore& kvs, Vbid id) {
2866+
expected = 1;
2867+
if (deleteItem) {
2868+
expected--;
2869+
}
2870+
2871+
auto checkKVS = [dropAtEnd, expected, &target](KVStore& kvs, Vbid id) {
28282872
auto [status, dropped] = kvs.getDroppedCollections(id);
28292873
ASSERT_TRUE(status);
28302874
EXPECT_TRUE(dropped.empty());
@@ -2842,7 +2886,7 @@ void CollectionsDcpPersistentOnly::resurrectionTest(bool dropAtEnd) {
28422886
} else {
28432887
auto [success, stats] = kvs.getCollectionStats(*fileHandle, target);
28442888
EXPECT_TRUE(success);
2845-
EXPECT_EQ(1, stats.itemCount);
2889+
EXPECT_EQ(expected, stats.itemCount);
28462890
EXPECT_EQ(7, stats.highSeqno);
28472891
}
28482892
};
@@ -2855,20 +2899,56 @@ void CollectionsDcpPersistentOnly::resurrectionTest(bool dropAtEnd) {
28552899
ASSERT_TRUE(replicaKVS);
28562900
checkKVS(*replicaKVS, replicaVB);
28572901

2858-
// MB-42272: skip check for magma
2859-
if (dropAtEnd && !(isFullEviction() && isMagma())) {
2902+
// MB-42272: skip check for magma full eviction, most variants don't pass
2903+
if ((isFullEviction() && isMagma())) {
2904+
return;
2905+
}
2906+
2907+
if (dropAtEnd) {
28602908
EXPECT_EQ(0, vb->getNumItems());
28612909
} else {
2862-
EXPECT_EQ(1, vb->getNumItems());
2910+
EXPECT_EQ(expected, vb->getNumItems());
28632911
}
28642912
}
28652913

28662914
TEST_P(CollectionsDcpPersistentOnly, create_drop_create_same_id) {
2867-
resurrectionTest(false);
2915+
resurrectionTest(false, false, false);
2916+
}
2917+
2918+
TEST_P(CollectionsDcpPersistentOnly, create_drop_create_same_id_update) {
2919+
resurrectionTest(false, true, false);
28682920
}
28692921

28702922
TEST_P(CollectionsDcpPersistentOnly, create_drop_create_same_id_end_dropped) {
2871-
resurrectionTest(true);
2923+
resurrectionTest(true, false, false);
2924+
}
2925+
2926+
TEST_P(CollectionsDcpPersistentOnly,
2927+
create_drop_create_same_id_end_dropped_update) {
2928+
resurrectionTest(true, true, false);
2929+
}
2930+
2931+
TEST_P(CollectionsDcpPersistentOnly, create_drop_create_same_id_delete) {
2932+
resurrectionTest(false, false, true);
2933+
}
2934+
2935+
TEST_P(CollectionsDcpPersistentOnly, create_drop_create_same_id_update_delete) {
2936+
resurrectionTest(false, true, true);
2937+
}
2938+
2939+
TEST_P(CollectionsDcpPersistentOnly,
2940+
create_drop_create_same_id_end_dropped_delete) {
2941+
resurrectionTest(true, false, true);
2942+
}
2943+
2944+
TEST_P(CollectionsDcpPersistentOnly,
2945+
create_drop_create_same_id_end_dropped_update_delete) {
2946+
if (isMagma()) {
2947+
// Test currently crashes with an underflow and will be fixed as part of
2948+
// MB-42272
2949+
GTEST_SKIP();
2950+
}
2951+
resurrectionTest(true, true, true);
28722952
}
28732953

28742954
void CollectionsDcpPersistentOnly::resurrectionStatsTest(

0 commit comments

Comments
 (0)