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

[JS-13] Copy global model when accessing it from local tablet #14

Merged
merged 5 commits into from
Dec 27, 2017

Conversation

wynot12
Copy link
Contributor

@wynot12 wynot12 commented Dec 26, 2017

Resolves #13

This PR

  • adds an option in ET Table's get methods for users to specify whether to copy the value or not
  • If copy is set true, ET puts the operation to the queue that handles remote operations, where values are serialized & deserialized (a performance issue may arise as described in Optimize local access routine of update and get operation #11).

Copy link
Contributor

@yunseong yunseong left a comment

Choose a reason for hiding this comment

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

Thanks for the PR. I've left a comment to understand how it works. Could you take a look? Thanks!

@@ -293,7 +296,7 @@ public void putIfAbsentNoReply(final K key, @Nonnull final V value) {
remoteIdOptional = remoteIdWithLock.getKey();
try {
// execute operation in local
if (!remoteIdOptional.isPresent()) {
if (!copy && !remoteIdOptional.isPresent()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you help me understand how values are copied when copy is true? Is it enough to put the request to a queue for handling remote operations?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

They're copied through encoding and decoding.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

And the operation should go into the queue not to get undetermined states by concurrent updates.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This way of copying have unnecessary overhead.
Issue #11.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the explanation. It looks working well; we may have to keep an eye on #11, though.
I will merge it.

@@ -293,7 +296,7 @@ public void putIfAbsentNoReply(final K key, @Nonnull final V value) {
remoteIdOptional = remoteIdWithLock.getKey();
try {
// execute operation in local
if (!remoteIdOptional.isPresent()) {
if (!copy && !remoteIdOptional.isPresent()) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the explanation. It looks working well; we may have to keep an eye on #11, though.
I will merge it.

@yunseong yunseong merged commit 58f24f4 into master Dec 27, 2017
@yunseong yunseong deleted the copy-global-model-from-local-tablet branch December 27, 2017 04:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants