Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add flush operation to KVAO #1206

Merged
merged 1 commit into from
Dec 31, 2016
Merged

Add flush operation to KVAO #1206

merged 1 commit into from
Dec 31, 2016

Conversation

superkhau
Copy link
Contributor

@superkhau superkhau commented Dec 31, 2016 •
edited

Used to delete all keys (and values) associated to the current model.

Connect to https://github.com/strongloop-internal/scrum-loopback/issues/1379

cc @bajtos

@superkhau superkhau self-assigned this Dec 31, 2016
@superkhau superkhau force-pushed the add-flush-op-to-kvao branch 2 times, most recently from 77748d7 to 7dbea89 Compare December 31, 2016 01:56
@simonhoibm
Add flush operation to KVAO
f38709b 
Used to delete all keys (and values) associated to the current model.
@superkhau superkhau mentioned this pull request Dec 31, 2016
Merged
@superkhau
Copy link
Contributor Author

@slnode test please

This was referenced Dec 31, 2016
Closed
Merged
@superkhau
Copy link
Contributor Author

@slnode test please

1 similar comment
@superkhau
Copy link
Contributor Author

@slnode test please

@superkhau
Copy link
Contributor Author

Oracle is failing due to executing ./run-tests on windows -- it is a known failure and unrelated to this PR.

@superkhau superkhau merged commit 79ec421 into master Dec 31, 2016
@superkhau superkhau removed the #review label Dec 31, 2016
@superkhau superkhau deleted the add-flush-op-to-kvao branch December 31, 2016 06:43
@superkhau superkhau added this to the Sprint 26 milestone Dec 31, 2016
@bajtos
Copy link
Member

bajtos commented Jan 2, 2017

@superkhau I am rather unhappy that addition of a new API was landed without any peer review :(

I personally find the new method name flush confusing and ambiguous.

Quoting google result:

cleanse (something, especially a toilet) by causing large quantities of water to pass through it.
"she flushed the loo"

  • (of a toilet) be cleansed by flushing.
    "Cally heard the toilet flush"
  • remove or dispose of (an object or substance) by flushing.
    "I flushed the pills down the lavatory"
  • cause (a liquid) to flow through something.
    "0.3 ml of saline is gently flushed through the tube"

Can we use a better name please, for example one of removeAllKeys, deleteAllKeys, clearAllKeys, clearDatabase, deleteAll?

Considering that lib/dao.js provides a similar method under the names remove, deleteAll and destroyAll, I think deleteAll may be the best choice.

@ritch thoughts?

bajtos
bajtos reviewed Jan 4, 2017
View reviewed changes
lib/connectors/kv-memory.js
@@ -201,6 +201,12 @@ KeyValueMemoryConnector.prototype.disconnect = function(callback) {
process.nextTick(callback);
};

KeyValueMemoryConnector.prototype.flush =
function(modelName, options, callback) {
this._store = Object.create(null);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is duplicating the code from constructor, I am worried that these two places may get out of sync soon. Please extract a shared private method, e.g. KeyValueMemoryConnector.prototype._resetStore().

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I forgot to submit my comments yesterday. Based on our last discussion, this comment is no longer relevant, as flush will be deleting only keys of a single model, i.e. the implementation will be different.

lib/connectors/kv-memory.js
KeyValueMemoryConnector.prototype.flush =
function(modelName, options, callback) {
this._store = Object.create(null);
callback();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

process.nextTick(callback)?

test/kvao/flush.suite.js

module.exports = function(dataSourceFactory, connectorCapabilities) {
var supportsFlushOperation =
connectorCapabilities.supportsFlushOperation !== false;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The way how I envision connectorCapabilities is to provide backwards compatibility, so that we don't have to upgrade all downstream connectors when we add a new operation to juggler. To achieve that, connectors must explicitly set supportsFlushOperation to true to enable this test suite.

var supportsFlushOperation =
  !!connectorCapabilities.supportsFlushOperation;

@superkhau
Copy link
Contributor Author

superkhau commented Jan 4, 2017 •
edited

...I am rather unhappy that addition of a new API was landed without any peer review :(

As discussed with @bajtos over hangouts, the change was a blocker and needed landing to verify the other KV PRs (and he was on vacation and I didn't know when he was coming back). Anyways, it's resolved and we will be changing flush to deleteAll + various other changes, which I'll link here at a later time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bajtos bajtos bajtos left review comments

Assignees

@superkhau superkhau

Labels
feature
Projects
None yet
Milestone
Sprint 26
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@superkhau @bajtos @simonhoibm