MB-23590: Update del_with_meta to accept a value (xattrs) 56/76256/5
authorJim Walker <jim@couchbase.com>
Tue, 4 Apr 2017 09:37:40 +0000 (10:37 +0100)
committerDave Rigby <daver@couchbase.com>
Thu, 6 Apr 2017 15:03:13 +0000 (15:03 +0000)
Clients need to be able to perform del_with_meta with an xattr value
i.e. delete the document, but leave the xattrs.

This commit adds the simple functionality of setting a value but does
not add more extensive functionality where we need to consider pruning
user xattrs from the deleted document.

This commit is also flawed in that a subsequent set with cas following
the delete may succeed when it should not as the del_with_meta(xattr)
leaves the StoredValue in the HT marked as isDeleted where a
del_with_meta would remove it from the HT, giving a different result
to the subsequent set.

Change-Id: I3f6d50ab96d60724a24d33abe405f2f4df069480
Reviewed-on: http://review.couchbase.org/76256
Tested-by: Build Bot <build@couchbase.com>
Reviewed-by: Dave Rigby <daver@couchbase.com>
src/ep_engine.cc
src/stored-value.h
src/vbucket.cc
tests/ep_test_apis.cc
tests/ep_test_apis.h
tests/ep_testsuite_xdcr.cc

index f8c2b13..6e498ae 100644 (file)
@@ -5367,11 +5367,13 @@ ENGINE_ERROR_CODE EventuallyPersistentEngine::deleteWithMeta(
     uint8_t opcode = request->message.header.request.opcode;
     uint16_t vbucket = ntohs(request->message.header.request.vbucket);
     uint64_t cas = ntohll(request->message.header.request.cas);
-
+    uint32_t bodylen = ntohl(request->message.header.request.bodylen);
     uint32_t flags = request->message.body.flags;
     uint32_t expiration = ntohl(request->message.body.expiration);
     uint64_t seqno = ntohll(request->message.body.seqno);
     uint64_t metacas = ntohll(request->message.body.cas);
+    uint8_t datatype = request->message.header.request.datatype;
+    size_t vallen = bodylen - nkey - extlen;
 
     bool skipConflictResolution = false;
     GenerateCas generateCas = GenerateCas::No;
@@ -5403,16 +5405,35 @@ ENGINE_ERROR_CODE EventuallyPersistentEngine::deleteWithMeta(
     uint64_t bySeqno = 0;
     ENGINE_ERROR_CODE ret = ENGINE_SUCCESS;
     try {
-        ret = deleteWithMeta(vbucket,
-                             key,
-                             {metacas, seqno, flags, time_t(expiration)},
-                             cas,
-                             &bySeqno,
-                             cookie,
-                             skipConflictResolution,
-                             GenerateBySeqno::Yes,
-                             generateCas,
-                             emd);
+        if (vallen) {
+            // A delete with a value
+            const uint8_t* value = keyPtr + nkey;
+            ret = setWithMeta(vbucket,
+                              key,
+                              {value, vallen},
+                              {metacas, seqno, flags, time_t(expiration)},
+                              true /*isDeleted*/,
+                              datatype,
+                              cas,
+                              &bySeqno,
+                              cookie,
+                              skipConflictResolution,
+                              true /*allowExisting*/,
+                              GenerateBySeqno::Yes,
+                              generateCas,
+                              emd);
+        } else {
+            ret = deleteWithMeta(vbucket,
+                                 key,
+                                 {metacas, seqno, flags, time_t(expiration)},
+                                 cas,
+                                 &bySeqno,
+                                 cookie,
+                                 skipConflictResolution,
+                                 GenerateBySeqno::Yes,
+                                 generateCas,
+                                 emd);
+        }
     } catch (const std::bad_alloc&) {
         return sendErrorResponse(response,
                                  PROTOCOL_BINARY_RESPONSE_ENOMEM,
index 7190f15..5743d40 100644 (file)
@@ -628,7 +628,7 @@ protected:
           exptime(itm.getExptime()),
           flags(itm.getFlags()),
           datatype(itm.getDataType()),
-          deleted(false),
+          deleted(itm.isDeleted()),
           newCacheItem(true),
           isOrdered(isOrdered),
           stale(false),
index 80df954..4a24c74 100644 (file)
@@ -937,8 +937,10 @@ ENGINE_ERROR_CODE VBucket::setWithMeta(Item& itm,
                 return ENGINE_EWOULDBLOCK;
             }
 
-            if (!(conflictResolver->resolve(
-                        *v, itm.getMetaData(), itm.getDataType(), false))) {
+            if (!(conflictResolver->resolve(*v,
+                                            itm.getMetaData(),
+                                            itm.getDataType(),
+                                            itm.isDeleted()))) {
                 ++stats.numOpsSetMetaResolutionFailed;
                 return ENGINE_KEY_EEXISTS;
             }
index 3ecfc85..7394ce8 100644 (file)
@@ -402,11 +402,18 @@ ENGINE_ERROR_CODE del(ENGINE_HANDLE *h, ENGINE_HANDLE_V1 *h1, const char *key,
                       cas, vbucket, mut_info);
 }
 
-void del_with_meta(ENGINE_HANDLE *h, ENGINE_HANDLE_V1 *h1, const char *key,
-                   const size_t keylen, const uint32_t vb,
-                   ItemMetaData *itemMeta, uint64_t cas_for_delete,
-                   uint32_t options, const void *cookie,
-                   const std::vector<char>& nmeta) {
+void del_with_meta(ENGINE_HANDLE* h,
+                   ENGINE_HANDLE_V1* h1,
+                   const char* key,
+                   const size_t keylen,
+                   const uint32_t vb,
+                   ItemMetaData* itemMeta,
+                   uint64_t cas_for_delete,
+                   uint32_t options,
+                   const void* cookie,
+                   const std::vector<char>& nmeta,
+                   protocol_binary_datatype_t datatype,
+                   const std::vector<char>& value) {
     int blen = 24;
     std::unique_ptr<char[]> ext(new char[30]);
     std::unique_ptr<ExtendedMetaData> emd;
@@ -426,9 +433,18 @@ void del_with_meta(ENGINE_HANDLE *h, ENGINE_HANDLE_V1 *h1, const char *key,
     }
 
     protocol_binary_request_header *pkt;
-    pkt = createPacket(PROTOCOL_BINARY_CMD_DEL_WITH_META, vb, cas_for_delete,
-                       ext.get(), blen, key, keylen, NULL, 0,
-                       PROTOCOL_BINARY_RAW_BYTES, nmeta.data(), nmeta.size());
+    pkt = createPacket(PROTOCOL_BINARY_CMD_DEL_WITH_META,
+                       vb,
+                       cas_for_delete,
+                       ext.get(),
+                       blen,
+                       key,
+                       keylen,
+                       value.data(),
+                       value.size(),
+                       datatype,
+                       nmeta.data(),
+                       nmeta.size());
 
     check(h1->unknown_command(h, cookie, pkt, add_response_set_del_meta, testHarness.doc_namespace) == ENGINE_SUCCESS,
           "Expected to be able to delete with meta");
index c6cd3a1..05d9050 100644 (file)
@@ -416,11 +416,18 @@ void add_with_meta(ENGINE_HANDLE *h, ENGINE_HANDLE_V1 *h1, const char *key,
                    const void* cookie = nullptr,
                    const std::vector<char>& nmeta = {});
 
-void del_with_meta(ENGINE_HANDLE *h, ENGINE_HANDLE_V1 *h1, const char *key,
-                   const size_t keylen, const uint32_t vb,
-                   ItemMetaData* itemMeta, uint64_t cas_for_delete = 0,
-                   uint32_t options = 0, const void *cookie = nullptr,
-                   const std::vector<char>& nmeta = {});
+void del_with_meta(ENGINE_HANDLE* h,
+                   ENGINE_HANDLE_V1* h1,
+                   const char* key,
+                   const size_t keylen,
+                   const uint32_t vb,
+                   ItemMetaData* itemMeta,
+                   uint64_t cas_for_delete = 0,
+                   uint32_t options = 0,
+                   const void* cookie = nullptr,
+                   const std::vector<char>& nmeta = {},
+                   protocol_binary_datatype_t datatype = 0,
+                   const std::vector<char>& value = {} /*optional value*/);
 
 void return_meta(ENGINE_HANDLE *h, ENGINE_HANDLE_V1 *h1, const char *key,
                  const size_t keylen, const char *val, const size_t vallen,
index 5bc1448..196aa51 100644 (file)
@@ -38,6 +38,34 @@ static void verifyLastMetaData(ItemMetaData imd) {
     checkeq(imd.flags, last_meta.flags, "Flags didn't match");
 }
 
+/**
+ * Create an XATTR document using the supplied string as the body
+ * @returns vector containing the body bytes
+ */
+static std::vector<char> createXattrValue(const std::string& body) {
+    cb::xattr::Blob blob;
+
+    //Add a few XAttrs
+    blob.set(to_const_byte_buffer("user"),
+             to_const_byte_buffer("{\"author\":\"bubba\"}"));
+    blob.set(to_const_byte_buffer("_sync"),
+             to_const_byte_buffer("{\"cas\":\"0xdeadbeefcafefeed\"}"));
+    blob.set(to_const_byte_buffer("meta"),
+             to_const_byte_buffer("{\"content-type\":\"text\"}"));
+
+    auto xattr_value = blob.finalize();
+
+    // append body to the xattrs and store in data
+    std::vector<char> data;
+    std::copy(xattr_value.buf, xattr_value.buf + xattr_value.len,
+              std::back_inserter(data));
+    std::copy(body.c_str(), body.c_str() + body.size(),
+              std::back_inserter(data));
+
+    return data;
+}
+
+
 // Testcases //////////////////////////////////////////////////////////////////
 
 static enum test_result test_get_meta(ENGINE_HANDLE *h, ENGINE_HANDLE_V1 *h1)
@@ -335,25 +363,7 @@ static enum test_result test_get_meta_with_delete(ENGINE_HANDLE *h, ENGINE_HANDL
 static enum test_result test_get_meta_with_xattr(ENGINE_HANDLE* h, ENGINE_HANDLE_V1* h1)
 {
     const char* key = "get_meta_key";
-    cb::xattr::Blob blob;
-
-    //Add a few XAttrs
-    blob.set(to_const_byte_buffer("user"),
-             to_const_byte_buffer("{\"author\":\"bubba\"}"));
-    blob.set(to_const_byte_buffer("_sync"),
-             to_const_byte_buffer("{\"cas\":\"0xdeadbeefcafefeed\"}"));
-    blob.set(to_const_byte_buffer("meta"),
-             to_const_byte_buffer("{\"content-type\":\"text\"}"));
-
-    auto xattr_value = blob.finalize();
-
-    //Now, append user data to the xattrs and store the data
-    std::string value_data("test_expiry_value");
-    std::vector<char> data;
-    std::copy(xattr_value.buf, xattr_value.buf + xattr_value.len,
-              std::back_inserter(data));
-    std::copy(value_data.c_str(), value_data.c_str() + value_data.length(),
-              std::back_inserter(data));
+    std::vector<char> data = createXattrValue({"test_expiry_value"});
 
     const void* cookie = testHarness.create_cookie();
 
@@ -1257,26 +1267,9 @@ static enum test_result test_set_with_meta_xattr(ENGINE_HANDLE* h,
                                                  ENGINE_HANDLE_V1* h1) {
     const char* key = "set_with_meta_xattr_key";
 
-    // Create a XATTR document with a JSON body
-    cb::xattr::Blob blob;
-
-    //Add a few XAttrs
-    blob.set(to_const_byte_buffer("user"),
-             to_const_byte_buffer("{\"author\":\"bubba\"}"));
-    blob.set(to_const_byte_buffer("_sync"),
-             to_const_byte_buffer("{\"cas\":\"0xdeadbeefcafefeed\"}"));
-    blob.set(to_const_byte_buffer("meta"),
-             to_const_byte_buffer("{\"content-type\":\"text\"}"));
-
-    auto xattr_value = blob.finalize();
-
-    // Now, append JSON user-data to the xattrs and store the data
-    std::string value_data(R"({"json":"yes"})");
-    std::vector<char> data;
-    std::copy(xattr_value.buf, xattr_value.buf + xattr_value.len,
-              std::back_inserter(data));
-    std::copy(value_data.c_str(), value_data.c_str() + value_data.length(),
-              std::back_inserter(data));
+    // Create XATTR doc with JSON body
+    std::string value_data = R"({"json":"yes"})";
+    std::vector<char> data = createXattrValue(value_data);
 
     const void* cookie = testHarness.create_cookie();
 
@@ -1342,32 +1335,40 @@ static enum test_result test_set_with_meta_xattr(ENGINE_HANDLE* h,
 
 static enum test_result test_delete_with_meta_xattr(ENGINE_HANDLE* h,
                                                     ENGINE_HANDLE_V1* h1) {
+    const char* key1 = "delete_with_meta_xattr_key1";
 
-    const char* key = "delete_with_meta_xattr_key";
-    const char* value = "somevalue";
     const void* cookie = testHarness.create_cookie();
 
-    //store a value
-    item* i = nullptr;
-    checkeq(ENGINE_SUCCESS,
-            store(h, h1, nullptr, OPERATION_SET, key, value, &i),
-            "Failed set.");
+    // Create XATTR doc with a JSON body
+    // In practice a del_with_meta should come along with only XATTR, but
+    // the command should work with a complete xattr/body blob
+    std::string body = R"({"key1":"value","key2":"value"})";
+    std::vector<char> data = createXattrValue(body);
 
-    h1->release(h, nullptr, i);
+    checkeq(ENGINE_SUCCESS,
+            store(h,
+                  h1,
+                  nullptr,
+                  OPERATION_SET,
+                  key1,
+                  body.data(),
+                  nullptr),
+            "Failed to store key1.");
 
     if (isPersistentBucket(h, h1)) {
         wait_for_flusher_to_settle(h, h1);
     }
 
-    check(get_meta(h, h1, key), "Expected to get meta");
-    checkeq(PROTOCOL_BINARY_RESPONSE_SUCCESS, last_status.load(), "Expected success");
+    // Get the metadata so we can build a del_with_meta
+    check(get_meta(h, h1, key1), "Failed get_meta(key1)");
+    checkeq(PROTOCOL_BINARY_RESPONSE_SUCCESS,
+            last_status.load(),
+            "last_status is not success for get_meta(key1)");
 
-    //init the meta data
-    ItemMetaData itm_meta;
-    itm_meta.revSeqno = last_meta.revSeqno;
-    itm_meta.cas = last_meta.cas;
-    itm_meta.exptime = last_meta.exptime;
-    itm_meta.flags = last_meta.flags;
+    // Init the meta data for a successful delete
+    ItemMetaData itm_meta = last_meta;
+    itm_meta.revSeqno = last_meta.revSeqno + 1; // +1 for seqno conflicts
+    itm_meta.cas = last_meta.cas + 1; // +1 for CAS conflicts
 
     int force = 0;
     if (strstr(testHarness.get_current_testcase()->cfg,
@@ -1375,35 +1376,70 @@ static enum test_result test_delete_with_meta_xattr(ENGINE_HANDLE* h,
         force = FORCE_ACCEPT_WITH_META_OPS;
     }
 
-    //delete with the same meta data
-    del_with_meta(h, h1, key, strlen(key), 0, &itm_meta,
-                  last_meta.cas, force, cookie, {});
-
-    checkeq(PROTOCOL_BINARY_RESPONSE_KEY_EEXISTS, last_status.load(),
-            "Expected the delete with meta to fail");
-
-    // Only enable XATTR
+    // Now enable XATTR
     testHarness.set_datatype_support(cookie, PROTOCOL_BINARY_DATATYPE_XATTR);
 
-    //delete with the same meta data
-    del_with_meta(h, h1, key, strlen(key), 0, &itm_meta, last_meta.cas, force,
-                  cookie, {});
+    // Now delete with a value (marked with XATTR)
+    del_with_meta(h,
+                  h1,
+                  key1,
+                  strlen(key1),
+                  0, // vb
+                  &itm_meta,
+                  0, // cas
+                  force,
+                  cookie,
+                  {}, // nmeta
+                  PROTOCOL_BINARY_DATATYPE_XATTR,
+                  data);
+
+    /* @todo this should fail, but doesn't. We've just deleted the item, but
+    it remains in the hash-table (marked deleted), the subsequent set finds the
+    item and is happy to process this SET_CAS operation (returns success).
+    A delete with no body would of dropped it from the hash-table and the
+    SET_CAS would return enoent, delete_with_meta /should/ I think have the same
+    effect.
+    checkeq(ENGINE_KEY_ENOENT,
+            store(h,
+                  h1,
+                  nullptr,
+                  OPERATION_SET,
+                  key1,
+                  body.data(),
+                  nullptr,
+                  last_cas.load()),
+            "Failed to store key1.");
+    */
 
-    checkeq(PROTOCOL_BINARY_RESPONSE_KEY_EEXISTS, last_status.load(),
-            "Expected the delete with meta to fail");
+    checkeq(PROTOCOL_BINARY_RESPONSE_SUCCESS,
+            last_status.load(),
+            "Expected delete_with_meta(key1) to succeed");
 
-    if (isPersistentBucket(h, h1)) {
-        //evict the key
-        evict_key(h, h1, key);
+    // Verify the new value is as expected
+    item_info info;
+    item* itm = nullptr;
+    checkeq(ENGINE_SUCCESS,
+            get(h, h1, nullptr, &itm, key1, 0, DocStateFilter::AliveOrDeleted),
+            "Failed to get(key1)");
+
+    check(h1->get_item_info(h, nullptr, itm, &info),
+          "Failed get_item_info of key1");
+    checkeq(data.size(), info.value[0].iov_len, "Value length mismatch");
+    checkeq(0, memcmp(info.value[0].iov_base, data.data(), data.size()),
+           "New body mismatch");
+    checkeq(int(DocumentState::Deleted), int(info.document_state),
+          "document_state is not DocumentState::Deleted");
+
+    // The new value is XATTR with a JSON body (the del_w_meta should of
+    // noticed)
+    checkeq(int(PROTOCOL_BINARY_DATATYPE_XATTR | PROTOCOL_BINARY_DATATYPE_JSON),
+            int(info.datatype),
+            "datatype isn't JSON and XATTR");
 
-        //delete with the same meta data again. This should result in a
-        //bg fetch
-        del_with_meta(h, h1, key, strlen(key), 0, &itm_meta,
-                      last_meta.cas, force, cookie, {});
+    h1->release(h, nullptr, itm);
 
-        checkeq(PROTOCOL_BINARY_RESPONSE_KEY_EEXISTS, last_status.load(),
-                "Expected delete with meta to fail with EEXISTS");
-    }
+    // @todo implement test for the deletion of a value that has xattr using
+    // a delete that has none (i.e. non-xattr/!spock client)
 
     testHarness.destroy_cookie(cookie);