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

Support update() with conditions #76

Closed
akudiyar opened this issue Oct 29, 2020 · 10 comments
Closed

Support update() with conditions #76

akudiyar opened this issue Oct 29, 2020 · 10 comments
Labels
feature A new functionality wontfix This will not be worked on

Comments

@akudiyar
Copy link
Contributor

Use case: customer wants to update a single field value in a large sharded space with some conditions. For example, change statuses of all orders with status "PAYMENT_RECEIVED" to "CLOSED".

Proposed API variants:
a) Allow update() method to receive conditions instead of key parts:

    crud.update('test_space', {{'=', 'status', 'PAYMENT_RECEIVED'}}, {{'=', 'status', 'CLOSED'}})

b) Add a new method updateall() (the method name may be different):

    crud.updateall('test_space', {{'=', 'status', 'PAYMENT_RECEIVED'}}, {{'=', 'status', 'CLOSED'}})

The same variants may be used for the upsert() operation.

@knazarov
Copy link
Contributor

I'm against extending the API to have methods that are created for the bulk update of whole spaces. This has the ability to lock the whole instance for a long time.

@akudiyar
Copy link
Contributor Author

Big selects have the same ability, and delete operation has been already supporting conditions. It is a matter of a convenient API and not forcing customers to write boilerplate code.

Consider the case when we substitute UPDATE .. WHERE SQL operation with a CRUD analog. Should we load data one by one from a large space to the client or maybe updating it in place is better?

@knazarov
Copy link
Contributor

That's why we will incur the same limitation we have for selects as we have in Data Grid. There will be a limit on how many rows a select can scan before returning with an error. Unbound selects shouldn't be allowed.

Yes, I believe it's better to pull data to the client and then update it in batches. It will keep the database operational at least. Doing an update that touches the whole data set will just freeze the database.

If you want to do an update of around 1-10 items, then pulling them to the client should not be a problem. If you want to update the whole table, then you've clearly picked the wrong way to implement it.

@knazarov
Copy link
Contributor

Updating whole datasets should be done by a background task.

@akudiyar
Copy link
Contributor Author

akudiyar commented Oct 29, 2020

If you mean "asynchronous" task which reports events about its state, it is a good case and may be supported in the connectors. So the users may wait or not until the task finishes, which depends on the business scenario. In this case, it seems like an implementation detail, and the update API may look like the proposed above.

Btw, the delete() operation has the same API now.

@akudiyar
Copy link
Contributor Author

If you want to do an update of around 1-10 items, then pulling them to the client should not be a problem. If you want to update the whole table, then you've clearly picked the wrong way to implement it.

It looks like "select for update" and I am afraid it is less efficient than the built-in in-place update.

@knazarov
Copy link
Contributor

The delete API doesn't have the same signature. It doesn't have the condition as in select. You can only delete by primary key. And it's a deliberate choice.

I already told you that we won't support mass updates from the update function. It can block the event loop for a long time and we don't want that.

@akudiyar
Copy link
Contributor Author

But what do you think about the mass updates in an asynchronous task which returns a future and produces events? Batch update using small portions of data may be implemented in CRUD this way.

@knazarov
Copy link
Contributor

The current API is stateless and doesn't support long-running tasks. And the basic CRUD operations won't be stateful by design. This is a deliberate choice.

In the future, it is possible that we will make some sort of background tasks, but I will resist it unless there is a solid argument for why they are useful.

For now, use "select for update". It should solve most of your problems.

@Totktonada
Copy link
Member

We discussed it with the product team and decided to don't implement it here. Sorry, Alexey.

@Totktonada Totktonada added wontfix This will not be worked on and removed teamP labels Jan 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feature A new functionality wontfix This will not be worked on
Projects
None yet
Development

No branches or pull requests

4 participants