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

WaitForPendingUpdate method #1

Closed
5 of 7 tasks
curquiza opened this issue Apr 8, 2020 · 11 comments
Closed
5 of 7 tasks

WaitForPendingUpdate method #1

curquiza opened this issue Apr 8, 2020 · 11 comments

Comments

@curquiza
Copy link
Member

curquiza commented Apr 8, 2020

We want to provide a method in the MeiliSearch SDKs to wait for the asynchronous update.

Really useful in the testing part instead of using sleep methods everwhere.

Naming

We chose the name WaitForPendingUpdate for this method.
Except for the JS: getUpdateSync because of JS convention.

The parameters should be named as updateId, interval and timeout.

Details of the method

This method must have a break condition.

This method is a loop that checks the status of the update every interval until a timeout.
Both parameters must be customizable by the user.

If it's possible with the language, both parameters should be optional and the method should set default values if they are not defined by the user.
If it's not, like in Golang, another function (like a "wrapper") could be provided with default parameters.

If the method times out, it should return a TimeOut Exception.

Example in PHP

public function waitForPendingUpdate($update_id, $timeout_in_ms = 2000, $interval_in_ms = 10)
{
    $timeout_temp = 0;
    while ($timeout_in_ms > $timeout_temp) {
        $res = $this->getUpdateStatus($update_id);
        if ('enqueued' != $res['status']) {
            return $res;
        }
        $timeout_temp += $interval_in_ms;
        usleep(1000 * $interval_in_ms);
    }
    throw new TimeOutException();
}

Testing part

At least, we should provide the following tests:

  • Test that works with default values
  • Test that works with custom interval and timeout
  • Test that returns an exception because of timeout: set a tiny value to timeout.

Steps

Do it in:

@curquiza
Copy link
Member Author

After discussing with @eskombro, we decided to choose these default values for interval and timeout:

  • interval: 50ms
  • timeout: 5 seconds

@bidoubiwa
Copy link
Contributor

I'm working on this right now!

@bidoubiwa
Copy link
Contributor

bidoubiwa commented Apr 23, 2020

After some research

Except for the JS: getUpdateSync because of JS convention.

The JS version will also be called waitForPendingUpdate because the Sync version implies that no wait or then is needed which is not the case.
Creating a sync version of a async function is not possible in javascript.

@bidoubiwa
Copy link
Contributor

I have removed all the sleeps in all the javascript tests and replaced them with

await client.getIndex(index.uid).waitForPendingUpdate(updateId)

Thanks to this function I'm now sure that the process will be resolved before continuing. No more arbitrary sleeps that are not reliable at all!

And bonus: It is so much faster!
Screenshot 2020-04-23 at 19 16 12

Screenshot 2020-04-23 at 19 10 05

@bidoubiwa
Copy link
Contributor

Should be updated for JS as done

@eskombro
Copy link
Member

eskombro commented Jun 1, 2020

After discussing this implementation with @tpayet , it seems that it is not necessarily the best idea to have 'updateId' as a parameter for the waitForPendingUpdate method.

With this method signature, the user needs to:

  1. Make a modification to some documents or settings
  2. Retrieve the response object sent by MeiliSearch (the json body).
  3. Inside that json, look for the updateId field
  4. Pass the value of that field as a parameter to waitForPendingUpdate method

It would be way easier and logical to receive the "update" json object, and pass it directly to the waitForPendingUpdate

  1. Make a modification to some documents or settings
  2. Pass the json body as a parameter to waitForPendingUpdate method

Example:

some_documents = [{ "name": "eskombro" }]
update = index.add_documents(some_documents)
wait_for_pending_update(update)

or even:

some_documents = [{ "name": "eskombro" }]
wait_for_pending_update(index.add_documents(some_documents))

What do you think @bidoubiwa and @curquiza ?

@bidoubiwa
Copy link
Contributor

Yes, that makes sense! It does not make the usage of it confusing and it makes it easier.

@curquiza
Copy link
Member Author

curquiza commented Jun 2, 2020

Like that!

Another possibility is to return an Update object = an instance of the Update class which implements the wait_for_pending_update method to write:

index.add_documents(some_documents).wait_for_pending_update()

In general, it's better to return an object (I mean an instance of a class with attributes and methods) than just a JSON-like as we do now.
The Update class could have a lot of convenient methods and attributes like:

  • updateId
  • getStatus
  • isProcessed?
  • hasFailed?
  • isPending?
  • waitForPendingUpdate
  • when MeiliSearch will handle warning: raisesWarning?
  • and so on...

But it might involves more implementation 😉

@eskombro
Copy link
Member

eskombro commented Jun 2, 2020

@curquiza I really like your idea!

But as you say, this implies a lot of refactor for some of the SDK's and seems like a bigger issue. (we can create a new discussion if you want).

But we agree in both visions that updateId shouldn't be the parameter of waitForPendingUpdate, but the whole 'object', if I am not wrong?

@curquiza
Copy link
Member Author

curquiza commented Jun 2, 2020

But we agree in both visions that updateId shouldn't be the parameter of waitForPendingUpdate, but the whole 'object', if I am not wrong?

For me, having the updateId as a parameter it's not a big and bad issue, but I agree with you: I would rather have the solution you suggested that the current one. If someone has time to implement/correct that in all the SDKs, that's cool.
My suggestion is a little bit complex for now.

@curquiza
Copy link
Member Author

This method is already implemented everywhere except in the Swift and Rust SDK where issues are opened! Closing this general issue then!

bors bot added a commit to meilisearch/meilisearch-go that referenced this issue Jun 22, 2021
150: Rewrite client r=alallema a=alallema

**Description**
Start of SDK go modification:
- Rewriting of the Client that implements this method:
	- Index(uid string)
	- GetIndex(indexID string)
	- GetAllIndexes()
	- CreateIndex(config IndexConfig)
	- GetOrCreateIndex(config IndexConfig)
	- GetKeys()
	- GetAllStats()
	- CreateDump()
	- GetDumpStatus(dumpUID string)
	- Version()
	- GetVersion()
	- Health()
	- IsHealthy()
	- DeleteIndex(uid string)
- Rewriting of the Index that implements this method:
	- FetchInfo()
	- FetchPrimaryKey()
	- ChangePrimaryKey()
	- Delete()
	- AddDocuments()
	- UpdateDocuments()
	- GetDocument()
	- GetDocuments()
	- DeleteOneDocument()
	- DeletesDocuments()
	- DeleteAllDocuments()
	- GetAllUpdateStatus()
	- GetUpdateStatus()
	- Search()
	- GetSettings()
	- UpdateSettings()
	- ResetSettings()
	- GetStats()
	- WaitForPendingUpdate()
	- DefaultWaitForPendingUpdate()
- Removing of intermediates interfaces
- Rewriting of sample and Readme for the new usage
- Add tests for every method

**To do**
- Client
	- [x] Rewrite Client
	- [x] Add Test to Client
- Index 
	- [x] Rewrite Index
	- [x] Add Tests to Index
	- [x] Adding test for `WaitForPendingUpdate()` Issue [related](meilisearch/integration-guides#1)
	- [x] Rewrite Document
	- [x] Add Tests to Document
	- [x] Rewrite Search
	- [x] Add Tests Search
	- [x] Rewrite Settings
	- [x] Add Tests Settings

Co-authored-by: alallema <amelie@meilisearch.com>
bors bot added a commit to meilisearch/meilisearch-go that referenced this issue Jun 22, 2021
150: Rewrite client r=alallema a=alallema

**Description**
Start of SDK go modification:
- Rewriting of the Client that implements this method:
	- Index(uid string)
	- GetIndex(indexID string)
	- GetAllIndexes()
	- CreateIndex(config IndexConfig)
	- GetOrCreateIndex(config IndexConfig)
	- GetKeys()
	- GetAllStats()
	- CreateDump()
	- GetDumpStatus(dumpUID string)
	- Version()
	- GetVersion()
	- Health()
	- IsHealthy()
	- DeleteIndex(uid string)
- Rewriting of the Index that implements this method:
	- FetchInfo()
	- FetchPrimaryKey()
	- ChangePrimaryKey()
	- Delete()
	- AddDocuments()
	- UpdateDocuments()
	- GetDocument()
	- GetDocuments()
	- DeleteOneDocument()
	- DeletesDocuments()
	- DeleteAllDocuments()
	- GetAllUpdateStatus()
	- GetUpdateStatus()
	- Search()
	- GetSettings()
	- UpdateSettings()
	- ResetSettings()
	- GetStats()
	- WaitForPendingUpdate()
	- DefaultWaitForPendingUpdate()
- Removing of intermediates interfaces
- Rewriting of sample and Readme for the new usage
- Add tests for every method

**To do**
- Client
	- [x] Rewrite Client
	- [x] Add Test to Client
- Index 
	- [x] Rewrite Index
	- [x] Add Tests to Index
	- [x] Adding test for `WaitForPendingUpdate()` Issue [related](meilisearch/integration-guides#1)
	- [x] Rewrite Document
	- [x] Add Tests to Document
	- [x] Rewrite Search
	- [x] Add Tests Search
	- [x] Rewrite Settings
	- [x] Add Tests Settings

Co-authored-by: alallema <amelie@meilisearch.com>
This was referenced Apr 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants