Refactor/kvao flush to deleteAll #1214

superkhau · 2017-01-06T02:42:07Z

Aggregate pull request for refactoring kvao.flush to kvao.deleteAll. Implementation consists of 3 parts:

delete
deleteAll
Fall back deleteAll

superkhau · 2017-01-06T07:51:57Z

@rmg I think something is wrong with CI, PTAL. Could be NPM though because I see npm ERR! 404 no such package available : inherits in the logs, could be something wrong with internal registry.

superkhau · 2017-01-06T07:52:04Z

@slnode test please

bajtos · 2017-01-06T08:51:17Z

lib/kvao/delete-all.js

+  if (typeof connector.deleteAll === 'function') {
+    connector.deleteAll(this.modelName, options, callback);
+  } else if (typeof connector.delete === 'function') {
+    console.warn('falling back to unoptimized `deleteAll`');


This will print a ton of warnings in a real app. Please use debug instead.

bajtos · 2017-01-06T08:54:07Z

lib/kvao/delete-all.js

+    connector.deleteAll(this.modelName, options, callback);
+  } else if (typeof connector.delete === 'function') {
+    console.warn('falling back to unoptimized `deleteAll`');
+    let it = connector.iterateKeys(this.modelName, {});


I think we should pass options instead of {} as the second arg.

bajtos · 2017-01-06T08:56:53Z

lib/kvao/delete-all.js

+  } else if (typeof connector.delete === 'function') {
+    console.warn('falling back to unoptimized `deleteAll`');
+    let it = connector.iterateKeys(this.modelName, {});
+    asyncIterators.toArray(it, function(err, keys) {


If you have 100k keys in your store, this will create an array of 100k, which seems like a huge waste of memory to me.

Is there a reason against iterating the async-iterator directly? See how .keys() is implemented in lib/kvao/keys.js.

This is what I am thinking about:

var iter = connector.iterateKeys(this.modelName, {}); var keys = []; iter.next(onNextKey); function onNextKey(err, key) { if (err) return callback(err); if (key === undefined) return callback(); connector.delete(self.modelName, key, options, onDeleted); } function onDeleted(err) { if (err) return callback(err); iter.next(onNextKey); }

Also since the code is more that a couple of lines, please move it to an new internal method, e.g. iterateAndDelete.

bajtos · 2017-01-06T08:57:50Z

lib/kvao/delete-all.js

+      }, callback);
+    });
+  } else {
+    console.warn('connector does not support `deleteAll`');


This should return an Error with statusCode=501.

bajtos · 2017-01-06T08:58:18Z

lib/kvao/delete.js

+  assert(typeof key === 'string' && key, 'key must be a non-empty string');
+
+  callback = callback || utils.createPromiseCallback();
+  this.getConnector().delete(this.modelName, key, options, callback);


Check that the connector provides delete method and report a descriptive 501 error otherwise.

bajtos · 2017-01-06T08:59:41Z

package.json

    "eslint": "^3.11.1",
    "eslint-config-loopback": "^6.0.0",
    "mocha": "^2.1.0",
    "should": "^8.0.2"
  },
  "dependencies": {
    "async": "~1.0.0",
+    "async-iterators": "^0.2.2",


I'd rather not depend on pre-1.0 versions in production. We are already implementing our own iteration in keys, I think we should do the same in the fall-back version of deleteAll (see my earlier comment for details).

I simply moved this from a devDep to a dep. It's actually not needed as a full on dep anymore with your suggested changes for deleteAll, so removing this as it's unused now.

bajtos · 2017-01-06T09:02:55Z

test/kvao/_helpers.js

+
+  if (excludeFromConnector) {
+    // `delete connector.deleteAll` doesn't work
+    dataSource.connector[excludeFromConnector] = undefined;


This is not the way how we force a datasource to use fall-back (unoptimized) methods.

See memory connector tests for the correct solution: https://github.com/strongloop/loopback-datasource-juggler/blob/66c54a96467b39609fec07f41b6d8018c5fc3819/test/memory.test.js#L887-L906.

The idea is to run the shared test suite twice for the built-in kv-memory connector. Once we run against the "full" connector, the second time we delete optimized functions from the connector instance after the datasource was created.

Th change in this file should be reverted.

bajtos · 2017-01-06T09:04:56Z

test/kvao/delete-all-fallback.suite.js

+const should = require('should');
+
+module.exports = function(dataSourceFactory, connectorCapabilities) {
+  // override injected data source factory


See my comment above, the decision fallback vs regular deleteAll is made by the connector. There is no point to test how connectors implementing optimized deleteAll are handling the fall-back variant, because the fall-back variant will be never invoked.

This whole file should be deleted and test/kv-memory.test.js should be modified to run the shared test suite twice.

bajtos · 2017-01-06T09:05:50Z

test/kvao/delete-all.suite.js

+      return helpers.givenKeys(CacheItem, ['key1', 'key2'])
+        .then(() => helpers.givenKeys(AnotherModel, ['key3', 'key4']))
+        .then(() => CacheItem.deleteAll())
+        .then(() => CacheItem.keys())


As I commented in your earlier pull request, there is no need to verify that CacheItem.deleteAll deletes CacaheItem keys - that has been already verified by the previous test.

rmg · 2017-01-06T16:29:15Z

@slnode test please

bajtos

Looks mostly good, see few minor comments below. No need for further review, unless you feel so.

bajtos · 2017-01-09T14:23:30Z

lib/kvao/delete-all.js

+    debug('falling back to unoptimized `deleteAll`');
+    iterateAndDelete(this.modelName, connector, options, callback);
+  } else {
+    debug('connector does not support `deleteAll`');


I think this is not entirely correct. How about Connector does not support deleting key/values. (Also notice the error message is a proper sentence, starting with a capital letter and ending with a dot. I don't mind the missing dot, but I believe we are starting error messages with a capital letter in other places.)

bajtos · 2017-01-09T14:24:25Z

lib/kvao/delete-all.js

+  } else {
+    debug('connector does not support `deleteAll`');
+    process.nextTick(function() {
+      var err = new Error('connector does not support `deleteAll`');


Ditto, new Error('Connector does not support deleting key/values').

(Feel free to use a different wording for "key/values", just stay consistent in the error message and the debug log.)

bajtos · 2017-01-09T14:25:32Z

lib/kvao/delete-all.js

+  return callback.promise;
+};
+
+function iterateAndDelete(modelName, connector, options, callback) {


Nitpick: I would personally change the order of arguments from (modelName, connector,...) to (connector, modelName, ...), Start with the thing that's changing the least (connector remains the same for the lifetime of a datasource), follow with things that change more often.

bajtos · 2017-01-09T14:27:26Z

lib/kvao/delete.js

+  if (typeof connector.delete === 'function') {
+    connector.delete(this.modelName, key, options, callback);
+  } else {
+    debug('connector does not support `delete`');


I think it may be better to use the same message I proposed for "deleteAll" above, i.e. Connector does not support deleting key/values.

bajtos · 2017-01-09T14:27:29Z

lib/kvao/delete.js

+  } else {
+    debug('connector does not support `delete`');
+    process.nextTick(function() {
+      var err = new Error('connector does not support `delete`');


- Rename `flush` to `deleteAll` - Add `delete` - Detect `delete/deleteAll` before running downstream test suites - Fall back to unoptimized `deleteAll` when connector does not support `deleteAll` but supports `delete` - Return 501 for connectors not supporting `delete` or `deleteAll`

superkhau · 2017-01-09T23:02:38Z

@slnode test please

superkhau · 2017-01-09T23:56:13Z

@slnode test please

superkhau · 2017-01-10T02:31:46Z

Created follow up issue for long term Oracle fail at loopbackio/loopback-connector-oracle#99. Merging as everything else is green.

superkhau added the feature label Jan 6, 2017

superkhau self-assigned this Jan 6, 2017

superkhau requested a review from bajtos January 6, 2017 02:42

superkhau added the #review label Jan 6, 2017

superkhau added this to the Sprint 26 milestone Jan 6, 2017

This was referenced Jan 6, 2017

Add kvao delete #1213

Closed

Use KVAO connector supported deleteAll or fall back to non-atomic deleteAll #1212

Closed

Rename KVAO.flush to deleteAll #1211

Closed

superkhau force-pushed the refactor/kvao-flush branch 4 times, most recently from 5e1615d to b998dd4 Compare January 6, 2017 04:28

superkhau changed the title ~~Refactor/kvao flush~~ Refactor/kvao flush to deleteAll Jan 6, 2017

superkhau force-pushed the refactor/kvao-flush branch 3 times, most recently from ad9c691 to 6c3d63b Compare January 6, 2017 07:39

superkhau force-pushed the refactor/kvao-flush branch from 6c3d63b to 43aa4c8 Compare January 6, 2017 08:48

bajtos suggested changes Jan 6, 2017

View reviewed changes

cgole removed the #review label Jan 7, 2017

superkhau force-pushed the refactor/kvao-flush branch 3 times, most recently from 0faeed3 to 52877bf Compare January 7, 2017 02:17

bajtos approved these changes Jan 9, 2017

View reviewed changes

superkhau force-pushed the refactor/kvao-flush branch from 52877bf to c387580 Compare January 9, 2017 22:36

superkhau force-pushed the refactor/kvao-flush branch from c387580 to 2320df1 Compare January 9, 2017 22:53

superkhau merged commit 6f2a075 into master Jan 10, 2017

superkhau deleted the refactor/kvao-flush branch January 10, 2017 02:31

This was referenced Jan 10, 2017

Remove flush op flag loopbackio/loopback-connector-kv-redis#27

Merged

Configurable mapname loopbackio/loopback-connector-kv-extreme-scale#12

Merged

Detect deleteAll support in KVAO tests #1218

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor/kvao flush to deleteAll #1214

Refactor/kvao flush to deleteAll #1214

superkhau commented Jan 6, 2017 •

edited

superkhau commented Jan 6, 2017 •

edited

superkhau commented Jan 6, 2017

bajtos Jan 6, 2017

bajtos Jan 6, 2017

bajtos Jan 6, 2017

bajtos Jan 6, 2017

bajtos Jan 6, 2017

bajtos Jan 6, 2017

superkhau Jan 7, 2017

bajtos Jan 6, 2017

bajtos Jan 6, 2017

bajtos Jan 6, 2017

rmg commented Jan 6, 2017

bajtos left a comment

bajtos Jan 9, 2017

bajtos Jan 9, 2017

bajtos Jan 9, 2017

bajtos Jan 9, 2017

bajtos Jan 9, 2017

superkhau commented Jan 9, 2017

superkhau commented Jan 9, 2017

superkhau commented Jan 10, 2017

Refactor/kvao flush to deleteAll #1214

Refactor/kvao flush to deleteAll #1214

Conversation

superkhau commented Jan 6, 2017 • edited

superkhau commented Jan 6, 2017 • edited

superkhau commented Jan 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rmg commented Jan 6, 2017

bajtos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

superkhau commented Jan 9, 2017

superkhau commented Jan 9, 2017

superkhau commented Jan 10, 2017

superkhau commented Jan 6, 2017 •

edited

superkhau commented Jan 6, 2017 •

edited