From 634574ed9f7066b1fa060a1119ef879668c715c9 Mon Sep 17 00:00:00 2001 From: Jan Date: Fri, 28 Jul 2017 12:08:37 +0200 Subject: [PATCH 1/3] don't mask errors with fake OOM messages (#2872) --- arangod/RocksDBEngine/RocksDBCollection.cpp | 6 +- arangod/RocksDBEngine/RocksDBIndexFactory.cpp | 180 ++++++++---------- arangod/RocksDBEngine/RocksDBVPackIndex.cpp | 16 +- 3 files changed, 90 insertions(+), 112 deletions(-) diff --git a/arangod/RocksDBEngine/RocksDBCollection.cpp b/arangod/RocksDBEngine/RocksDBCollection.cpp index b05aba5c13..28f1f625a1 100644 --- a/arangod/RocksDBEngine/RocksDBCollection.cpp +++ b/arangod/RocksDBEngine/RocksDBCollection.cpp @@ -381,7 +381,7 @@ static std::shared_ptr findIndex( if (!value.isString()) { // Compatibility with old v8-vocindex. - THROW_ARANGO_EXCEPTION(TRI_ERROR_OUT_OF_MEMORY); + THROW_ARANGO_EXCEPTION_MESSAGE(TRI_ERROR_INTERNAL, "invalid index type definition"); } std::string tmp = value.copyString(); @@ -1369,7 +1369,7 @@ RocksDBOperationResult RocksDBCollection::insertDocument( for (std::shared_ptr const& idx : _indexes) { innerRes.reset(idx->insert(trx, revisionId, doc, false)); - // in case of no-memory, return immediately + // in case of OOM return immediately if (innerRes.is(TRI_ERROR_OUT_OF_MEMORY)) { return innerRes; } @@ -1433,7 +1433,7 @@ RocksDBOperationResult RocksDBCollection::removeDocument( Result tmpres = idx->remove(trx, revisionId, doc, false); resInner.reset(tmpres); - // in case of no-memory, return immediately + // in case of OOM return immediately if (resInner.is(TRI_ERROR_OUT_OF_MEMORY)) { return resInner; } diff --git a/arangod/RocksDBEngine/RocksDBIndexFactory.cpp b/arangod/RocksDBEngine/RocksDBIndexFactory.cpp index f13c602336..0984ce80b6 100644 --- a/arangod/RocksDBEngine/RocksDBIndexFactory.cpp +++ b/arangod/RocksDBEngine/RocksDBIndexFactory.cpp @@ -57,42 +57,38 @@ static int ProcessIndexFields(VPackSlice const definition, TRI_ASSERT(builder.isOpenObject()); std::unordered_set fields; - try { - VPackSlice fieldsSlice = definition.get("fields"); - builder.add(VPackValue("fields")); - builder.openArray(); - if (fieldsSlice.isArray()) { - // "fields" is a list of fields - for (auto const& it : VPackArrayIterator(fieldsSlice)) { - if (!it.isString()) { - return TRI_ERROR_BAD_PARAMETER; - } - - StringRef f(it); - - if (f.empty() || (create && f == StaticStrings::IdString)) { - // accessing internal attributes is disallowed - return TRI_ERROR_BAD_PARAMETER; - } - - if (fields.find(f) != fields.end()) { - // duplicate attribute name - return TRI_ERROR_BAD_PARAMETER; - } - - fields.insert(f); - builder.add(it); + VPackSlice fieldsSlice = definition.get("fields"); + builder.add(VPackValue("fields")); + builder.openArray(); + if (fieldsSlice.isArray()) { + // "fields" is a list of fields + for (auto const& it : VPackArrayIterator(fieldsSlice)) { + if (!it.isString()) { + return TRI_ERROR_BAD_PARAMETER; } - } - if (fields.empty() || (numFields > 0 && (int)fields.size() != numFields)) { - return TRI_ERROR_BAD_PARAMETER; - } + StringRef f(it); - builder.close(); - } catch (...) { - return TRI_ERROR_OUT_OF_MEMORY; + if (f.empty() || (create && f == StaticStrings::IdString)) { + // accessing internal attributes is disallowed + return TRI_ERROR_BAD_PARAMETER; + } + + if (fields.find(f) != fields.end()) { + // duplicate attribute name + return TRI_ERROR_BAD_PARAMETER; + } + + fields.insert(f); + builder.add(it); + } } + + if (fields.empty() || (numFields > 0 && (int)fields.size() != numFields)) { + return TRI_ERROR_BAD_PARAMETER; + } + + builder.close(); return TRI_ERROR_NO_ERROR; } @@ -293,77 +289,65 @@ int RocksDBIndexFactory::enhanceIndexDefinition(VPackSlice const definition, } TRI_ASSERT(enhanced.isEmpty()); + + VPackObjectBuilder b(&enhanced); + current = definition.get("id"); + uint64_t id = 0; + if (current.isNumber()) { + id = current.getNumericValue(); + } else if (current.isString()) { + id = basics::StringUtils::uint64(current.copyString()); + } + if (id > 0) { + enhanced.add("id", VPackValue(std::to_string(id))); + } + + if (create && !isCoordinator) { + if (!definition.hasKey("objectId")) { + enhanced.add("objectId", + VPackValue(std::to_string(TRI_NewTickServer()))); + } + } + + enhanced.add("type", VPackValue(Index::oldtypeName(type))); + int res = TRI_ERROR_INTERNAL; - try { - VPackObjectBuilder b(&enhanced); - current = definition.get("id"); - uint64_t id = 0; - if (current.isNumber()) { - id = current.getNumericValue(); - } else if (current.isString()) { - id = basics::StringUtils::uint64(current.copyString()); - } - if (id > 0) { - enhanced.add("id", VPackValue(std::to_string(id))); + switch (type) { + case Index::TRI_IDX_TYPE_PRIMARY_INDEX: + case Index::TRI_IDX_TYPE_EDGE_INDEX: { + break; } - if (create && !isCoordinator) { - if (!definition.hasKey("objectId")) { - enhanced.add("objectId", - VPackValue(std::to_string(TRI_NewTickServer()))); - } + case Index::TRI_IDX_TYPE_GEO1_INDEX: + res = EnhanceJsonIndexGeo1(definition, enhanced, create); + break; + + case Index::TRI_IDX_TYPE_GEO2_INDEX: + res = EnhanceJsonIndexGeo2(definition, enhanced, create); + break; + + case Index::TRI_IDX_TYPE_HASH_INDEX: + res = EnhanceJsonIndexHash(definition, enhanced, create); + break; + + case Index::TRI_IDX_TYPE_SKIPLIST_INDEX: + res = EnhanceJsonIndexSkiplist(definition, enhanced, create); + break; + + case Index::TRI_IDX_TYPE_PERSISTENT_INDEX: + res = EnhanceJsonIndexPersistent(definition, enhanced, create); + break; + + case Index::TRI_IDX_TYPE_FULLTEXT_INDEX: + res = EnhanceJsonIndexFulltext(definition, enhanced, create); + break; + + case Index::TRI_IDX_TYPE_UNKNOWN: + default: { + res = TRI_ERROR_BAD_PARAMETER; + break; } - // breaks lookupIndex() - /*else { - if (!definition.hasKey("objectId")) { - // objectId missing, but must be present - return TRI_ERROR_INTERNAL; - } - }*/ - - enhanced.add("type", VPackValue(Index::oldtypeName(type))); - - switch (type) { - case Index::TRI_IDX_TYPE_PRIMARY_INDEX: - case Index::TRI_IDX_TYPE_EDGE_INDEX: { - break; - } - - case Index::TRI_IDX_TYPE_GEO1_INDEX: - res = EnhanceJsonIndexGeo1(definition, enhanced, create); - break; - - case Index::TRI_IDX_TYPE_GEO2_INDEX: - res = EnhanceJsonIndexGeo2(definition, enhanced, create); - break; - - case Index::TRI_IDX_TYPE_HASH_INDEX: - res = EnhanceJsonIndexHash(definition, enhanced, create); - break; - - case Index::TRI_IDX_TYPE_SKIPLIST_INDEX: - res = EnhanceJsonIndexSkiplist(definition, enhanced, create); - break; - - case Index::TRI_IDX_TYPE_PERSISTENT_INDEX: - res = EnhanceJsonIndexPersistent(definition, enhanced, create); - break; - - case Index::TRI_IDX_TYPE_FULLTEXT_INDEX: - res = EnhanceJsonIndexFulltext(definition, enhanced, create); - break; - - case Index::TRI_IDX_TYPE_UNKNOWN: - default: { - res = TRI_ERROR_BAD_PARAMETER; - break; - } - } - - } catch (...) { - // TODO Check for different type of Errors - return TRI_ERROR_OUT_OF_MEMORY; } return res; diff --git a/arangod/RocksDBEngine/RocksDBVPackIndex.cpp b/arangod/RocksDBEngine/RocksDBVPackIndex.cpp index c9197a9c92..078b4fecd8 100644 --- a/arangod/RocksDBEngine/RocksDBVPackIndex.cpp +++ b/arangod/RocksDBEngine/RocksDBVPackIndex.cpp @@ -532,13 +532,10 @@ Result RocksDBVPackIndex::insertInternal(transaction::Methods* trx, std::vector elements; std::vector hashes; int res; - try { + { + // rethrow all types of exceptions from here... transaction::BuilderLeaser leased(trx); res = fillElement(*(leased.get()), revisionId, doc, elements, hashes); - } catch (basics::Exception const& ex) { - res = ex.code(); - } catch (...) { - res = TRI_ERROR_OUT_OF_MEMORY; } if (res != TRI_ERROR_NO_ERROR) { @@ -604,14 +601,11 @@ Result RocksDBVPackIndex::removeInternal(transaction::Methods* trx, std::vector hashes; int res; - try { + { + // rethrow all types of exceptions from here... transaction::BuilderLeaser leased(trx); res = fillElement(*(leased.get()), revisionId, doc, elements, hashes); - } catch (basics::Exception const& ex) { - res = ex.code(); - } catch (...) { - res = TRI_ERROR_OUT_OF_MEMORY; - } + } if (res != TRI_ERROR_NO_ERROR) { return IndexResult(res, this); From 9a0bc716d0175f2bcf9ffc061e0025aae03ea75e Mon Sep 17 00:00:00 2001 From: m0ppers Date: Fri, 28 Jul 2017 14:24:02 +0200 Subject: [PATCH 2/3] Do not allow replication to create/drop collections (#2898) In the cluster case the only one who is allowed to do this is the schmutz --- .../MMFiles/MMFilesRestReplicationHandler.cpp | 4 ++-- arangod/Replication/ContinuousSyncer.cpp | 2 +- arangod/Replication/InitialSyncer.cpp | 17 ++++++++++++++--- arangod/Replication/InitialSyncer.h | 7 ++++++- .../RocksDBRestReplicationHandler.cpp | 4 ++-- arangod/V8Server/v8-replication.cpp | 7 ++++++- js/server/modules/@arangodb/cluster.js | 2 +- 7 files changed, 32 insertions(+), 11 deletions(-) diff --git a/arangod/MMFiles/MMFilesRestReplicationHandler.cpp b/arangod/MMFiles/MMFilesRestReplicationHandler.cpp index 7745dc76c2..23cd2f3fd6 100644 --- a/arangod/MMFiles/MMFilesRestReplicationHandler.cpp +++ b/arangod/MMFiles/MMFilesRestReplicationHandler.cpp @@ -2893,7 +2893,7 @@ void MMFilesRestReplicationHandler::handleCommandMakeSlave() { std::string errorMsg = ""; { InitialSyncer syncer(_vocbase, &config, config._restrictCollections, - restrictType, false); + restrictType, false, false); res = TRI_ERROR_NO_ERROR; @@ -3014,7 +3014,7 @@ void MMFilesRestReplicationHandler::handleCommandSync() { MMFilesLogfileManager::instance()->waitForSync(5.0); InitialSyncer syncer(_vocbase, &config, restrictCollections, restrictType, - verbose); + verbose, false); std::string errorMsg = ""; diff --git a/arangod/Replication/ContinuousSyncer.cpp b/arangod/Replication/ContinuousSyncer.cpp index 2ba0c7d40d..e23a89dd6d 100644 --- a/arangod/Replication/ContinuousSyncer.cpp +++ b/arangod/Replication/ContinuousSyncer.cpp @@ -256,7 +256,7 @@ retry: try { InitialSyncer syncer( _vocbase, &_configuration, _configuration._restrictCollections, - _configuration._restrictType, _configuration._verbose); + _configuration._restrictType, _configuration._verbose, false); res = syncer.run(errorMsg, _configuration._incremental); diff --git a/arangod/Replication/InitialSyncer.cpp b/arangod/Replication/InitialSyncer.cpp index aea95b4249..20ccd220a2 100644 --- a/arangod/Replication/InitialSyncer.cpp +++ b/arangod/Replication/InitialSyncer.cpp @@ -65,7 +65,7 @@ InitialSyncer::InitialSyncer( TRI_vocbase_t* vocbase, TRI_replication_applier_configuration_t const* configuration, std::unordered_map const& restrictCollections, - std::string const& restrictType, bool verbose) + std::string const& restrictType, bool verbose, bool skipCreateDrop) : Syncer(vocbase, configuration), _progress("not started"), _restrictCollections(restrictCollections), @@ -77,7 +77,8 @@ InitialSyncer::InitialSyncer( _includeSystem(false), _chunkSize(configuration->_chunkSize), _verbose(verbose), - _hasFlushed(false) { + _hasFlushed(false), + _skipCreateDrop(skipCreateDrop) { if (_chunkSize == 0) { _chunkSize = (uint64_t)2 * 1024 * 1024; // 2 mb } else if (_chunkSize < 128 * 1024) { @@ -1132,6 +1133,10 @@ int InitialSyncer::handleCollection(VPackSlice const& parameters, } } else { // regular collection + if (_skipCreateDrop) { + setProgress("dropping " + collectionMsg + " skipped because of configuration"); + return TRI_ERROR_NO_ERROR; + } setProgress("dropping " + collectionMsg); int res = _vocbase->dropCollection(col, true, -1.0); @@ -1160,7 +1165,13 @@ int InitialSyncer::handleCollection(VPackSlice const& parameters, } } - std::string const progress = "creating " + collectionMsg; + std::string progress = "creating " + collectionMsg; + if (_skipCreateDrop) { + progress += " skipped because of configuration"; + setProgress(progress.c_str()); + return TRI_ERROR_NO_ERROR; + } + setProgress(progress.c_str()); int res = createCollection(parameters, &col); diff --git a/arangod/Replication/InitialSyncer.h b/arangod/Replication/InitialSyncer.h index bbb023c9fb..4af016dff2 100644 --- a/arangod/Replication/InitialSyncer.h +++ b/arangod/Replication/InitialSyncer.h @@ -98,7 +98,7 @@ class InitialSyncer : public Syncer { public: InitialSyncer(TRI_vocbase_t*, TRI_replication_applier_configuration_t const*, std::unordered_map const&, - std::string const&, bool verbose); + std::string const&, bool verbose, bool skipCreateDrop); ~InitialSyncer(); @@ -331,6 +331,11 @@ class InitialSyncer : public Syncer { static size_t const MaxChunkSize; + // in the cluster case it is a total NOGO to create or drop collections + // because this HAS to be handled in the schmutz. otherwise it forgets who + // the leader was etc. + bool _skipCreateDrop; + }; } diff --git a/arangod/RocksDBEngine/RocksDBRestReplicationHandler.cpp b/arangod/RocksDBEngine/RocksDBRestReplicationHandler.cpp index 85716edab1..df6d526841 100644 --- a/arangod/RocksDBEngine/RocksDBRestReplicationHandler.cpp +++ b/arangod/RocksDBEngine/RocksDBRestReplicationHandler.cpp @@ -1489,7 +1489,7 @@ void RocksDBRestReplicationHandler::handleCommandMakeSlave() { std::string errorMsg = ""; { InitialSyncer syncer(_vocbase, &config, config._restrictCollections, - restrictType, false); + restrictType, false, false); res = TRI_ERROR_NO_ERROR; @@ -1607,7 +1607,7 @@ void RocksDBRestReplicationHandler::handleCommandSync() { config._useCollectionId = useCollectionId; InitialSyncer syncer(_vocbase, &config, restrictCollections, restrictType, - verbose); + verbose, false); std::string errorMsg = ""; diff --git a/arangod/V8Server/v8-replication.cpp b/arangod/V8Server/v8-replication.cpp index 8a3ca03690..a03c3d540d 100644 --- a/arangod/V8Server/v8-replication.cpp +++ b/arangod/V8Server/v8-replication.cpp @@ -254,6 +254,11 @@ static void JS_SynchronizeReplication( verbose = TRI_ObjectToBoolean(object->Get(TRI_V8_ASCII_STRING("verbose"))); } + bool skipCreateDrop = false; + if (object->Has(TRI_V8_ASCII_STRING("skipCreateDrop"))) { + skipCreateDrop = TRI_ObjectToBoolean(object->Get(TRI_V8_ASCII_STRING("skipCreateDrop"))); + } + if (endpoint.empty()) { TRI_V8_THROW_EXCEPTION_PARAMETER(" must be a valid endpoint"); } @@ -319,7 +324,7 @@ static void JS_SynchronizeReplication( std::string errorMsg = ""; InitialSyncer syncer(vocbase, &config, restrictCollections, restrictType, - verbose); + verbose, skipCreateDrop); if (!leaderId.empty()) { syncer.setLeaderId(leaderId); } diff --git a/js/server/modules/@arangodb/cluster.js b/js/server/modules/@arangodb/cluster.js index e2aa78046a..eb10e88d06 100644 --- a/js/server/modules/@arangodb/cluster.js +++ b/js/server/modules/@arangodb/cluster.js @@ -545,7 +545,7 @@ function synchronizeOneShard (database, shard, planId, leader) { let startTime = new Date(); sy = rep.syncCollection(shard, { endpoint: ep, incremental: true, keepBarrier: true, - useCollectionId: false, leaderId: leader }); + useCollectionId: false, leaderId: leader, skipCreateDrop: true }); let endTime = new Date(); let longSync = false; if (endTime - startTime > 5000) { From ec7cc0059b3382478f8c6853311e1801a8daed2f Mon Sep 17 00:00:00 2001 From: Manuel B Date: Fri, 28 Jul 2017 15:19:14 +0200 Subject: [PATCH 3/3] change "parameters" to "bindVars" (#2772) * change "parameters" to "bindVars" * deprecate parameters --- arangod/V8Server/v8-vocbase.cpp | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/arangod/V8Server/v8-vocbase.cpp b/arangod/V8Server/v8-vocbase.cpp index 06d2a8c73e..6b4f089d1b 100644 --- a/arangod/V8Server/v8-vocbase.cpp +++ b/arangod/V8Server/v8-vocbase.cpp @@ -761,7 +761,8 @@ static void JS_ParseAql(v8::FunctionCallbackInfo const& args) { for (auto const& elem : parseResult.bindParameters) { bindVars->Set(i++, TRI_V8_STD_STRING((elem))); } - result->Set(TRI_V8_ASCII_STRING("parameters"), bindVars); + result->Set(TRI_V8_ASCII_STRING("parameters"), bindVars); // parameters is deprecated + result->Set(TRI_V8_ASCII_STRING("bindVars"), bindVars); } result->Set(TRI_V8_ASCII_STRING("ast"),