From 47600e1ea1d50e926a9c37e3ace926d1b2a6eca2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Simon=20Gr=C3=A4tzer?= Date: Thu, 26 Oct 2017 09:56:17 +0200 Subject: [PATCH] Changing replication factor in 3.2 (#3513) * Allow changing replicationFactor on coordinator * Fixing logic * Allowing change of replication factor * Additional input validation * grrr * Testing invalid inputs --- arangod/Cluster/ClusterInfo.cpp | 1 + arangod/VocBase/LogicalCollection.cpp | 23 ++ .../modules/@arangodb/arango-collection.js | 2 +- .../shell/shell-properties-cluster-spec.js | 208 ++++++++++++++++++ 4 files changed, 233 insertions(+), 1 deletion(-) create mode 100644 js/server/tests/shell/shell-properties-cluster-spec.js diff --git a/arangod/Cluster/ClusterInfo.cpp b/arangod/Cluster/ClusterInfo.cpp index cc1a9ee39f..eb0d5f5d71 100644 --- a/arangod/Cluster/ClusterInfo.cpp +++ b/arangod/Cluster/ClusterInfo.cpp @@ -1576,6 +1576,7 @@ int ClusterInfo::setCollectionPropertiesCoordinator( VPackBuilder temp; temp.openObject(); temp.add("waitForSync", VPackValue(info->waitForSync())); + temp.add("replicationFactor", VPackValue(info->replicationFactor())); info->getPhysical()->getPropertiesVPackCoordinator(temp); temp.close(); diff --git a/arangod/VocBase/LogicalCollection.cpp b/arangod/VocBase/LogicalCollection.cpp index 08fdcec393..0bd56b36b7 100644 --- a/arangod/VocBase/LogicalCollection.cpp +++ b/arangod/VocBase/LogicalCollection.cpp @@ -957,6 +957,26 @@ arangodb::Result LogicalCollection::updateProperties(VPackSlice const& slice, // ... probably a few others missing here ... WRITE_LOCKER(writeLocker, _infoLock); + + VPackSlice rf = slice.get("replicationFactor"); + if (!_isLocal && !rf.isNone()) { + if (!_distributeShardsLike.empty()) { + LOG_TOPIC(ERR, Logger::FIXME) << "Cannot change replicationFactor, please change " << _distributeShardsLike; + return Result(TRI_ERROR_FORBIDDEN, "Cannot change replicationFactor, " + "please change " + _distributeShardsLike); + } else if (_type == TRI_COL_TYPE_EDGE && _isSmart) { + LOG_TOPIC(ERR, Logger::FIXME) << "Changing replicationFactor not supported for smart edge collections"; + return Result(TRI_ERROR_NOT_IMPLEMENTED, "Changing replicationFactor " + "not supported for smart edge collections"); + } else if (isSatellite()) { + LOG_TOPIC(ERR, Logger::FIXME) << "Satellite collection cannot have replicationFactor"; + return Result(TRI_ERROR_FORBIDDEN, "Satellite collection " + "cannot have replicationFactor"); + } + if (!rf.isInteger() || rf.getInt() <= 0 || rf.getUInt() > 10) { + return Result(TRI_ERROR_BAD_PARAMETER, "bad value replicationFactor"); + } + } // The physical may first reject illegal properties. // After this call it either has thrown or the properties are stored @@ -967,6 +987,9 @@ arangodb::Result LogicalCollection::updateProperties(VPackSlice const& slice, } _waitForSync = Helper::getBooleanValue(slice, "waitForSync", _waitForSync); + if (rf.isNumber()) { + _replicationFactor = rf.getNumber(); + } if (!_isLocal) { // We need to inform the cluster as well diff --git a/js/client/modules/@arangodb/arango-collection.js b/js/client/modules/@arangodb/arango-collection.js index 8bef03e7fe..69a0a9b34c 100644 --- a/js/client/modules/@arangodb/arango-collection.js +++ b/js/client/modules/@arangodb/arango-collection.js @@ -335,7 +335,7 @@ ArangoCollection.prototype.properties = function (properties) { 'numberOfShards': false, 'keyOptions': false, 'indexBuckets': true, - 'replicationFactor': false, + 'replicationFactor': true, 'distributeShardsLike': false, 'cacheEnabled': true }; diff --git a/js/server/tests/shell/shell-properties-cluster-spec.js b/js/server/tests/shell/shell-properties-cluster-spec.js new file mode 100644 index 0000000000..4ea13a52a9 --- /dev/null +++ b/js/server/tests/shell/shell-properties-cluster-spec.js @@ -0,0 +1,208 @@ +/*global describe, it, ArangoAgency, afterEach */ + +//////////////////////////////////////////////////////////////////////////////// +/// @brief cluster collection creation tests +/// +/// @file +/// +/// 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 Andreas Streichardt +/// @author Copyright 2017, ArangoDB GmbH, Cologne, Germany +//////////////////////////////////////////////////////////////////////////////// + +'use strict'; + +const expect = require('chai').expect; +const internal = require("internal"); +const errors = require('@arangodb').errors; +const db = require("@arangodb").db; + +const cn1 = "UnitTestPropertiesLeader"; +const cn2 = "UnitTestPropertiesFollower"; + +function checkReplicationFactor(name, fac) { + let current = ArangoAgency.get('Current/Collections/_system'); + let plan = ArangoAgency.get('Plan/Collections/_system'); + let collectionId = Object.values(plan.arango.Plan.Collections['_system']).reduce((result, collectionDef) => { + if (result) { + return result; + } + if (collectionDef.name === name) { + return collectionDef.id; + } + }, undefined); + + Object.values(current.arango.Current.Collections['_system'][collectionId]).forEach(entry => { + expect(entry.servers).to.have.lengthOf(fac); + }); +}; + +describe('Update collection properties', function() { + + beforeEach(function() { + db._useDatabase("_system"); + }); + + afterEach(function() { + db._useDatabase("_system"); + try { + db._drop(cn1); + } catch (e) {} + }); + + it('increase replication factor ', function() { + db._create(cn1, {replicationFactor: 1, numberOfShards: 2}, {waitForSyncReplication: true}); + + checkReplicationFactor(cn1, 1) + + const coll = db._collection(cn1); + + let props = coll.properties({replicationFactor: 2}); + expect(props.replicationFactor).to.equal(2); + + // FIXME: do not wait for a fixed time + internal.sleep(5); + + checkReplicationFactor(cn1, 2); + }); + + it('decrease replication factor ', function() { + db._create(cn1, {replicationFactor: 2, numberOfShards: 2}, {waitForSyncReplication: true}); + + checkReplicationFactor(cn1, 2) + + const coll = db._collection(cn1); + + let props = coll.properties({replicationFactor: 1}); + expect(props.replicationFactor).to.equal(1); + + internal.sleep(5); + + checkReplicationFactor(cn1, 1); + }); + + it('invalid replication factor', function() { + db._create(cn1, {replicationFactor: 2, numberOfShards: 2}, {waitForSyncReplication: true}); + + checkReplicationFactor(cn1, 2) + + try { + const coll = db._collection(cn1); + coll.properties({replicationFactor: -1}); + expect(false.replicationFactor).to.equal(true, + "Was able to update replicationFactor of follower"); + } catch(e) { + expect(e.errorNum).to.equal(errors.ERROR_BAD_PARAMETER.code); + } + + try { + const coll = db._collection(cn1); + coll.properties({replicationFactor: 100}); + expect(false.replicationFactor).to.equal(true, + "Was able to update replicationFactor of follower"); + } catch(e) { + expect(e.errorNum).to.equal(errors.ERROR_BAD_PARAMETER.code); + } + + try { + const coll = db._collection(cn1); + coll.properties({replicationFactor: "satellite"}); + expect(false.replicationFactor).to.equal(true, + "Was able to update replicationFactor of follower"); + } catch(e) { + expect(e.errorNum).to.equal(errors.ERROR_BAD_PARAMETER.code); + } + }); +}); + + +describe('Update collection properties with distributeShardsLike, ', function() { + + + beforeEach(function() { + db._useDatabase("_system"); + }); + + afterEach(function() { + db._useDatabase("_system"); + + try { + db._drop(cn2); + } catch (e) {} + + try { + db._drop(cn1); + } catch (e) {} + }); + + it('increase replication factor', function() { + db._create(cn1, {replicationFactor: 1, numberOfShards: 2}, {waitForSyncReplication: true}); + db._create(cn2, {distributeShardsLike: cn1}, {waitForSyncReplication: true}); + + checkReplicationFactor(cn1, 1); + checkReplicationFactor(cn2, 1); + + const leader = db._collection(cn1); + let props = leader.properties({replicationFactor: 2}); + expect(props.replicationFactor).to.equal(2); + + // FIXME: do not wait for a fixed time + internal.sleep(5); + + checkReplicationFactor(cn1, 2); + checkReplicationFactor(cn2, 2); + }); + + it('decrease replication factor', function() { + db._create(cn1, {replicationFactor: 2, numberOfShards: 2}, {waitForSyncReplication: true}); + db._create(cn2, {distributeShardsLike: cn1}, {waitForSyncReplication: true}); + + checkReplicationFactor(cn1, 2); + checkReplicationFactor(cn2, 2); + + const leader = db._collection(cn1); + + let props = leader.properties({replicationFactor: 1}); + expect(props.replicationFactor).to.equal(1); + + // FIXME: do not wait for a fixed time + internal.sleep(5); + + checkReplicationFactor(cn1, 1); + checkReplicationFactor(cn2, 1); + }); + + it('change replicationFactor of follower', function() { + db._create(cn1, {replicationFactor: 2, numberOfShards: 2}, {waitForSyncReplication: true}); + db._create(cn2, {distributeShardsLike: cn1}, {waitForSyncReplication: true}); + + checkReplicationFactor(cn1, 2); + checkReplicationFactor(cn2, 2); + + try { + const follower = db._collection(cn2); + follower.properties({replicationFactor: 1}); + expect(false.replicationFactor).to.equal(true, + "Was able to update replicationFactor of follower"); + } catch(e) { + expect(e.errorNum).to.equal(errors.ERROR_FORBIDDEN.code); + } + }); +}); \ No newline at end of file