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.

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

Delete data on row.js mutation calls #207

Merged
merged 13 commits into from
Jun 28, 2018
Merged

Conversation

muraliQlogic
Copy link
Contributor

@muraliQlogic muraliQlogic commented Jun 23, 2018

Fixes #151 (it's a good idea to open an issue first for discussion)

  • Tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Jun 23, 2018
@codecov
Copy link

codecov bot commented Jun 23, 2018

Codecov Report

Merging #207 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@          Coverage Diff          @@
##           master   #207   +/-   ##
=====================================
  Coverage     100%   100%           
=====================================
  Files          10     10           
  Lines        1260   1266    +6     
=====================================
+ Hits         1260   1266    +6
Impacted Files Coverage Δ
src/row.js 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e77c3c9...503d0fe. Read the comment docs.

@ghost ghost assigned sduskis Jun 25, 2018
@sduskis
Copy link
Contributor

sduskis commented Jun 25, 2018

There are two cases where this.bigtable.request is called. Those cases also need to delete data.

@sduskis
Copy link
Contributor

sduskis commented Jun 25, 2018

Also, 'create', 'delete', and 'save' methods all need the same logic.

@sduskis sduskis changed the title fix for Issue#151 Delete data on row.js mutation calls Jun 25, 2018
@sduskis
Copy link
Contributor

sduskis commented Jun 25, 2018

@theacodes, we have an object in row.js that's the result of a read from Cloud Bigtable, but also can perform additional mutations against the Cloud Bigtable service.

At this point, we either have to cleanup the read state when mutations are performed, or we have to design a different solution.

We're hoping to reach Beta soon, so we have to decide what to do ASAP.

@theacodes
Copy link

I lean on @JustinBeckwith and @kinwa91 along with the maintainers of this repo to pick what's idiomatic in Node.js

@ghost ghost assigned JustinBeckwith Jun 25, 2018
@sduskis sduskis requested a review from stephenplusplus June 25, 2018 19:17
@sduskis
Copy link
Contributor

sduskis commented Jun 26, 2018

@JustinBeckwith and @kinwa91 do you have any guidance about this PR?

@JustinBeckwith
Copy link
Contributor

Apologies - I don't entirely understand the question. Can you elaborate?

@sduskis
Copy link
Contributor

sduskis commented Jun 27, 2018

Issue #149 / #151 is caused by an "active record" idiom employed in row.js. A Row is both the result of a read, and a mechanism by which mutation RPCs are sent to Cloud Bigtable. We maintain a member variable (data) in Row, which is not updated when ever a mutation occurs.

We can fix this in the following ways, and we need to figure out which one is the best (idiomatic, and least painful for the user) approach :

  1. Remove all state from Row, and provide a more primitive object to the user for the result of reads
  2. Clear out Row's data whenever a mutation occurs
  3. If data is set and a mutation occurs, update the data variable with the latest values.

We opted for option 2 in this PR, but one of the other options may be "better." We're looking for guidance on direction.

@stephenplusplus
Copy link
Contributor

This is familiar to the metadata property on all of our "ServiceObject" instances, e.g. a GCS File:

bucket.getFiles(function(err, files) {
  files[0].metadata // metadata record
})

metadata is not meant to be guaranteed accurate as your app ticks on; only at the time of fetching. So, we expose a getMetadata() to allow the user to refresh it, if they need to. At the same time, the internal property metadata is updated.

I would advise a user that .data is only accurate at the time of a read. And if you can make the guarantee that it's accurate at the time of any mutation, even better (is it possible to do that without requiring a subsequent read?).

The issue in #151 is definitely a bug-- instead of extending the local cache of "data", can we just overwrite this.data?

@sduskis
Copy link
Contributor

sduskis commented Jun 27, 2018

@stephenplusplus, it's not possible to keep .data up to date, since there are too many variables, including server-state and the filters used for reading the data.

sduskis added 4 commits June 27, 2018 18:15
In read, data is now set to the resulting row's data.  In mutation methods, clearing the map instead of deleting it.
@sduskis sduskis merged commit 7c19034 into googleapis:master Jun 28, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants