Bug fix 3.4/babies unknown shard (#9872)

* release version 3.4.8

* [3.4] fix agency lockup when removing 404-ed callbacks (#9839)

* fix non-deterministic occurrences of "document not found" in sharded
collections with custom sharding and multi-document lookup

* updated CHANGELOG

* fix error code handling

* Update CHANGELOG

CHANGELOG

@@ -1,6 +1,9 @@
-v3.4.8 (2019-08-29)
+v3.4.8 (2019-09-XX)
 -------------------
 
+* Fixed non-deterministic occurrences of "document not found" errors in sharded 
+  collections with custom shard keys (i.e. non-`_key`) and multi-document lookups.
+
 * Fixed issue #9798: limit with offset results in an infinite query that can't be
   cancelled.
 

arangod/Cluster/ClusterMethods.cpp

@@ -283,6 +283,16 @@ static void mergeResults(std::vector<std::pair<ShardID, VPackValueLength>> const
 ///        well
 ////////////////////////////////////////////////////////////////////////////////
 
+namespace {
+// velocypack representation of object
+// {"error":true,"errorMessage":"document not found","errorNum":1202}
+static const char* notFoundSlice = 
+  "\x14\x36\x45\x65\x72\x72\x6f\x72\x1a\x4c\x65\x72\x72\x6f\x72\x4d"
+  "\x65\x73\x73\x61\x67\x65\x52\x64\x6f\x63\x75\x6d\x65\x6e\x74\x20"
+  "\x6e\x6f\x74\x20\x66\x6f\x75\x6e\x64\x48\x65\x72\x72\x6f\x72\x4e"
+  "\x75\x6d\x29\xb2\x04\x03";
+} // namespace
+
 static void mergeResultsAllShards(std::vector<std::shared_ptr<VPackBuilder>> const& results,
                                   std::shared_ptr<VPackBuilder>& resultBody,
                                   std::unordered_map<int, size_t>& errorCounter,
@@ -291,12 +301,7 @@ static void mergeResultsAllShards(std::vector<std::shared_ptr<VPackBuilder>> con
   TRI_ASSERT(errorCounter.find(TRI_ERROR_ARANGO_DOCUMENT_NOT_FOUND) ==
              errorCounter.end());
   size_t realNotFound = 0;
-  VPackBuilder cmp;
+  
-  cmp.openObject();
-  cmp.add(StaticStrings::Error, VPackValue(true));
-  cmp.add(StaticStrings::ErrorNum, VPackValue(TRI_ERROR_ARANGO_DOCUMENT_NOT_FOUND));
-  cmp.close();
-  VPackSlice notFound = cmp.slice();
   resultBody->clear();
   resultBody->openArray();
   for (VPackValueLength currentIndex = 0; currentIndex < expectedResults; ++currentIndex) {
@@ -305,7 +310,14 @@ static void mergeResultsAllShards(std::vector<std::shared_ptr<VPackBuilder>> con
       VPackSlice oneRes = it->slice();
       TRI_ASSERT(oneRes.isArray());
       oneRes = oneRes.at(currentIndex);
-      if (basics::VelocyPackHelper::compare(oneRes, notFound, false) != 0) {
+      
+      int errorNum = TRI_ERROR_NO_ERROR;
+      VPackSlice errorNumSlice = oneRes.get(StaticStrings::ErrorNum);
+      if (errorNumSlice.isNumber()) {
+        errorNum = errorNumSlice.getNumber<int>();
+      }
+      if ((errorNum != TRI_ERROR_NO_ERROR && errorNum != TRI_ERROR_ARANGO_DOCUMENT_NOT_FOUND) ||
+          oneRes.hasKey(StaticStrings::KeyString)) {
         // This is the correct result
         // Use it
         resultBody->add(oneRes);
@@ -315,7 +327,7 @@ static void mergeResultsAllShards(std::vector<std::shared_ptr<VPackBuilder>> con
     }
     if (!foundRes) {
       // Found none, use NOT_FOUND
-      resultBody->add(notFound);
+      resultBody->add(VPackSlice(reinterpret_cast<uint8_t const*>(notFoundSlice)));
       realNotFound++;
     }
   }

tests/js/common/shell/shell-babies.js

@@ -0,0 +1,232 @@
+/*jshint globalstrict:false, strict:false, maxlen: 5000 */
+/*global fail, assertTrue, assertFalse, assertEqual, assertNotEqual, assertTypeOf */
+
+////////////////////////////////////////////////////////////////////////////////
+/// @brief test the babies interface
+///
+/// DISCLAIMER
+///
+/// Copyright 2010-2012 triagens GmbH, Cologne, Germany
+///
+/// Licensed under the Apache License, Version 2.0 (the "License");
+/// you may not use this file except in compliance with the License.
+/// You may obtain a copy of the License at
+///
+///     http://www.apache.org/licenses/LICENSE-2.0
+///
+/// Unless required by applicable law or agreed to in writing, software
+/// distributed under the License is distributed on an "AS IS" BASIS,
+/// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+/// See the License for the specific language governing permissions and
+/// limitations under the License.
+///
+/// Copyright holder is triAGENS GmbH, Cologne, Germany
+///
+/// @author Jan Steemann
+/// @author Copyright 2012, triAGENS GmbH, Cologne, Germany
+////////////////////////////////////////////////////////////////////////////////
+
+const jsunity = require("jsunity");
+const arangodb = require("@arangodb");
+const ERRORS = arangodb.errors;
+const db = arangodb.db;
+
+function BabiesStandardShardingSuite () {
+  'use strict';
+
+  const n = 50;
+  const cn = "UnitTestsCollectionBabies";
+  let c;
+
+  let keys = [];
+  for (let i = 0; i < n; ++i) {
+    keys.push("test" + i);
+  }
+
+  return {
+
+    setUp : function () {
+      db._drop(cn);
+      c = db._create(cn, { numberOfShards: 7 });
+      for (let i = 0; i < n; ++i) {
+        c.insert({ _key: "test" + i, value:  i });
+      }
+    },
+
+    tearDown : function () {
+      db._drop(cn);
+    },
+
+    testGetBabiesStandard : function () {
+      let docs = c.document(keys);
+      for (let i = 0; i < n; ++i) {
+        assertFalse(docs[i].hasOwnProperty("error"));
+        assertEqual(i, docs[i].value);
+        assertEqual("test" + i, docs[i]._key);
+      }
+    },
+    
+    testGetNonExistingBabiesStandard : function () {
+      let keys = [];
+      for (let i = 0; i < n; ++i) {
+        keys.push("peng" + i);
+      }
+      let docs = c.document(keys);
+      for (let i = 0; i < n; ++i) {
+        assertTrue(docs[i].error);
+        assertFalse(docs[i].hasOwnProperty("_key"));
+        assertEqual(ERRORS.ERROR_ARANGO_DOCUMENT_NOT_FOUND.code, docs[i].errorNum);
+      }
+    },
+    
+    testInsertBabiesStandard : function () {
+      c.truncate();
+      let docs = [];
+      for (let i = 0; i < n; ++i) {
+        docs.push({ _key: "test" + i, value: i });
+      }
+      docs = c.insert(docs);
+      for (let i = 0; i < n; ++i) {
+        assertFalse(docs[i].hasOwnProperty("error"));
+        assertEqual("test" + i, docs[i]._key);
+      }
+    },
+    
+    testRemoveBabiesStandard : function () {
+      let docs = c.remove(keys);
+      for (let i = 0; i < n; ++i) {
+        assertFalse(docs[i].hasOwnProperty("error"));
+        assertEqual("test" + i, docs[i]._key);
+      }
+    },
+    
+    testUpdateBabiesStandard : function () {
+      let docs = [];
+      for (let i = 0; i < n; ++i) {
+        docs.push({ value: i * 2 });
+      }
+      docs = c.update(keys, docs);
+      for (let i = 0; i < n; ++i) {
+        assertFalse(docs[i].hasOwnProperty("error"));
+        assertTrue(docs[i].hasOwnProperty("_oldRev"));
+        assertEqual("test" + i, docs[i]._key);
+      }
+    },
+    
+    testReplaceBabiesStandard : function () {
+      let docs = [];
+      for (let i = 0; i < n; ++i) {
+        docs.push({ value: i * 2 });
+      }
+      docs = c.replace(keys, docs);
+      for (let i = 0; i < n; ++i) {
+        assertFalse(docs[i].hasOwnProperty("error"));
+        assertTrue(docs[i].hasOwnProperty("_oldRev"));
+        assertEqual("test" + i, docs[i]._key);
+      }
+    },
+    
+
+  };
+}
+
+function BabiesCustomShardingSuite () {
+  'use strict';
+
+  const n = 50;
+  const cn = "UnitTestsCollectionBabies";
+  let c;
+  let keys;
+
+  return {
+
+    setUp : function () {
+      db._drop(cn);
+      c = db._create(cn, { numberOfShards: 7, shardKeys: ["value"] });
+      keys = [];
+      for (let i = 0; i < n; ++i) {
+        keys.push(c.insert({ value:  i })._key);
+      }
+    },
+
+    tearDown : function () {
+      db._drop(cn);
+    },
+
+    testGetBabiesCustom : function () {
+      let docs = c.document(keys);
+      for (let i = 0; i < n; ++i) {
+        assertFalse(docs[i].hasOwnProperty("error"));
+        assertEqual(i, docs[i].value);
+        assertEqual(keys[i], docs[i]._key);
+      }
+    },
+    
+    testGetNonExistingBabiesCustom : function () {
+      let keys = [];
+      for (let i = 0; i < n; ++i) {
+        keys.push("peng" + i);
+      }
+      let docs = c.document(keys);
+      for (let i = 0; i < n; ++i) {
+        assertTrue(docs[i].error);
+        assertFalse(docs[i].hasOwnProperty("_key"));
+        assertEqual(ERRORS.ERROR_ARANGO_DOCUMENT_NOT_FOUND.code, docs[i].errorNum);
+      }
+    },
+    
+    testInsertBabiesCustom : function () {
+      c.truncate();
+      let docs = [];
+      for (let i = 0; i < n; ++i) {
+        docs.push({ value: i });
+      }
+      docs = c.insert(docs);
+      for (let i = 0; i < n; ++i) {
+        assertFalse(docs[i].hasOwnProperty("error"));
+        assertTrue(docs[i].hasOwnProperty("_key"));
+      }
+    },
+    
+    testRemoveBabiesCustom : function () {
+      let docs = c.remove(keys);
+      for (let i = 0; i < n; ++i) {
+        assertFalse(docs[i].hasOwnProperty("error"));
+        assertEqual(keys[i], docs[i]._key);
+      }
+    },
+    
+    testUpdateBabiesCustom : function () {
+      let docs = [];
+      for (let i = 0; i < n; ++i) {
+        docs.push({ value: i, value2: i * 2 });
+      }
+      docs = c.update(keys, docs);
+      for (let i = 0; i < n; ++i) {
+        assertFalse(docs[i].hasOwnProperty("error"));
+        assertTrue(docs[i].hasOwnProperty("_oldRev"));
+        assertEqual(keys[i], docs[i]._key);
+      }
+    },
+    
+    testReplaceBabiesCustom : function () {
+      let docs = [];
+      for (let i = 0; i < n; ++i) {
+        docs.push({ value: i, value2: i * 2 });
+      }
+      docs = c.replace(keys, docs);
+      for (let i = 0; i < n; ++i) {
+        assertFalse(docs[i].hasOwnProperty("error"));
+        assertTrue(docs[i].hasOwnProperty("_oldRev"));
+        assertEqual(keys[i], docs[i]._key);
+      }
+    },
+    
+
+  };
+}
+
+jsunity.run(BabiesStandardShardingSuite);
+jsunity.run(BabiesCustomShardingSuite);
+
+return jsunity.done();