Skip to content

ARROW-1927: [Plasma] Add delete function#1427

Closed
JinHai-CN wants to merge 10 commits intoapache:masterfrom
JinHai-CN:plasma-delete
Closed

ARROW-1927: [Plasma] Add delete function#1427
JinHai-CN wants to merge 10 commits intoapache:masterfrom
JinHai-CN:plasma-delete

Conversation

@JinHai-CN
Copy link
Contributor

@JinHai-CN JinHai-CN commented Dec 17, 2017

pcmoritz
pcmoritz reviewed Dec 17, 2017
View reviewed changes
cpp/src/plasma/store.cc Outdated
Copy link
Contributor

@pcmoritz pcmoritz Dec 17, 2017

Choose a reason for hiding this comment

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

comment should be indented as much as the following line

robertnishihara
robertnishihara reviewed Dec 17, 2017
View reviewed changes
cpp/src/plasma/client.cc Outdated
Copy link
Contributor

@robertnishihara robertnishihara Dec 17, 2017

Choose a reason for hiding this comment

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

In client.h we should probably clarify the behavior of this method. In particular, if a client calls Delete, that does not guarantee that the object will be deleted. In particular, if the object is in use by another client, then the Delete call is a no-op, right? Can you mention this in the documentation for this method in client.h?

Copy link
Contributor Author

@JinHai-CN JinHai-CN Dec 17, 2017

Choose a reason for hiding this comment

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

No, it doesn't guarantee the object will be deleted for some reasons.

cpp/src/plasma/store.cc Outdated
Copy link
Contributor

@robertnishihara robertnishihara Dec 17, 2017

Choose a reason for hiding this comment

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

Given that we already have a delete_objects method, does it make sense to also have a delete_object method? Is the behavior of this method different from calling delete_objects on a vector of one object ID?

Copy link
Contributor Author

@JinHai-CN JinHai-CN Dec 17, 2017

Choose a reason for hiding this comment

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

To be honest, they are similar. But in delete_object will issue the error status to client if delete object failed. As for delete_objects used by evict objects function, I don't want to impact it too much.

pcmoritz
pcmoritz reviewed Dec 17, 2017
View reviewed changes
cpp/src/plasma/store.cc Outdated
Copy link
Contributor

@pcmoritz pcmoritz Dec 17, 2017

Choose a reason for hiding this comment

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

comment indentation

pcmoritz
pcmoritz reviewed Dec 17, 2017
View reviewed changes
cpp/src/plasma/store.cc Outdated
Copy link
Contributor

@pcmoritz pcmoritz Dec 17, 2017

Choose a reason for hiding this comment

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

comment indentation

pcmoritz
pcmoritz reviewed Dec 17, 2017
View reviewed changes
cpp/src/plasma/client.cc Outdated
Copy link
Contributor

@pcmoritz pcmoritz Dec 17, 2017

Choose a reason for hiding this comment

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

Shall we create a custom error for this so it can be caught in Python?

cc @wesm

pcmoritz
pcmoritz reviewed Dec 17, 2017
View reviewed changes
Copy link
Contributor

@pcmoritz pcmoritz left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Hey, this looks good, thanks for the contribution! Can you please also create a unit test that tests the following two scenarios:

  • make sure an error is raised if the object is in use
  • test the "normal" case where the object is not in use and make sure it was actually deleted (maybe by trying to create an object with the same object id)
  • test the case of an object getting deleted that doesn't exist yet.

@JinHai-CN
Copy link
Contributor Author

JinHai-CN commented Dec 17, 2017 via email

robertnishihara
robertnishihara reviewed Dec 20, 2017
View reviewed changes
Copy link
Contributor

@robertnishihara robertnishihara left a comment

Choose a reason for hiding this comment

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

Nice work! It could be done in a follow up PR, but we should expose this to the Python API and also add a test in https://github.com/apache/arrow/blob/master/python/pyarrow/tests/test_plasma.py

cpp/src/plasma/client.cc Outdated
Copy link
Contributor

@robertnishihara robertnishihara Dec 20, 2017

Choose a reason for hiding this comment

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

The error should probably just be "Object is in use."

Copy link
Contributor Author

@JinHai-CN JinHai-CN Dec 20, 2017

Choose a reason for hiding this comment

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

Yes, fixed

cpp/src/plasma/client.cc Outdated
Copy link
Contributor

@robertnishihara robertnishihara Dec 20, 2017

Choose a reason for hiding this comment

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

What do you think about not having the plasma store reply to the client? The client could just continue on. If it really wants to know whether the object was deleted or not, it can call Contains to check.

Are there use cases where you really want to know if the object was deleted or not?

Copy link
Contributor Author

@JinHai-CN JinHai-CN Dec 20, 2017

Choose a reason for hiding this comment

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

Providing a blocked API is the most common thinking. If delete operation failed, client also need to know why the delete operation failed and do the next step according to the reason.

Copy link
Contributor

@robertnishihara robertnishihara Dec 20, 2017

Choose a reason for hiding this comment

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

Can you describe the use case?

If you view the command as more of a "hint" that doesn't necessarily do anything but simply gives the plasma store more information that it can make use of, then a non-blocking API is natural.

Copy link
Contributor Author

@JinHai-CN JinHai-CN Dec 20, 2017

Choose a reason for hiding this comment

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

Yes, just update operation. And it can be implemented as: check contains, delete, check contains again to make it happen.
Currently, only two reasons: not sealed object and object is in use will make the DELETE operation failed. We can make it as non-blocking API. But when I look at other APIs such as CREATE, they also will provide the reply to the client. I think DELETE operation should follow the same way as CREATE operation.

Copy link
Contributor

@robertnishihara robertnishihara Dec 20, 2017

Choose a reason for hiding this comment

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

Ok, let's try out this approach and see if it's useful, though I think we should consider switching to "hint" semantics at some point.

cpp/src/plasma/eviction_policy.cc Outdated
Copy link
Contributor

@robertnishihara robertnishihara Dec 20, 2017

Choose a reason for hiding this comment

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

I think we also need to update the memory_used_ field.

Copy link
Contributor Author

@JinHai-CN JinHai-CN Dec 20, 2017

Choose a reason for hiding this comment

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

Yes, fixed.

@pcmoritz
Copy link
Contributor

pcmoritz commented Jan 10, 2018

+1 This looks good. The test failure looks unrelated (the node.js tests are failing a lot)

@pcmoritz pcmoritz closed this in f82b7e4 Jan 10, 2018
@robertnishihara
Copy link
Contributor

robertnishihara commented Jan 10, 2018

We should also expose this through Python and add Python tests.

@pcmoritz
Copy link
Contributor

pcmoritz commented Jan 10, 2018

Yeah, let's do this as a followup PR

@wesm
Copy link
Member

wesm commented Jan 11, 2018

@JinHai-CN could you let me know your JIRA id on the ASF JIRA so I can assign this issue to you?

@JinHai-CN
Copy link
Contributor Author

JinHai-CN commented Jan 15, 2018

@wesm ID: jinhai

@JinHai-CN JinHai-CN deleted the plasma-delete branch March 18, 2019 02:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@pcmoritz pcmoritz pcmoritz left review comments

@robertnishihara robertnishihara robertnishihara left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@JinHai-CN @pcmoritz @robertnishihara @wesm