Skip to content

Add "wait" version of each method calling asynchronous part of MeiliSearch #12

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

Closed
curquiza opened this issue Apr 28, 2020 · 4 comments
Closed
Labels
need discussion Need discussion to make a decision

Comments

@curquiza
Copy link
Member

curquiza commented Apr 28, 2020

Some actions in MeiliSearch are asynchronous (documents manipulation, settings...). The associated methods in the SDKs return information about the update with an enqueued status and we don't know when the action is processed.

We already provide, for most of the SDKs, the waitForPendingUpdate method (related #1). But it means this kind of usage (in ruby):

update = index.add_documents(...)
index.wait_for_pending_update(update['updateId'])

Suggestion 1

We could provide a "wait" version of each method for better user experience, for example:

index.add_documents_wait(...)

Naming

First of all, how can we name these methods?
A simple way would be to add a suffix to each method. I suggested wait without thinking about it a lot. So, I would like to read your ideas 🙂

Coding

Instead of re-coding all the methods, we could find a way to generate the version of each concerned method, because there are just the same but with a wait_for_pending_update method.
The goal is to avoid code duplication.
I haven't investigated this part yet.

Suggestion 2

index.add_documents(...).wait_for_pending_update()

Not sure if this solution is more user friendly than the other one. Investigation needed 😁

Coding

This solution would imply changing the return of all the current methods (big breaking). They should return an Update object instead of a JSON object containing information about the update. This Update object should contain the wait_for_pending_update method.

Suggestion 3

def add_documents(documents, primary_key = nil, wait_for_update = false)

This solution might be the easiest one to code, but problem with the Go!
A solution could be not to provide these methods for the Go language and wait for the demand/help of the community. Do not forget the WaitForPendingUpdate method is already available in the SDK Go.

Conclusion

Even if you would rather have other suggestions than suggestion 1, I really like having your suggestions of naming for the suggestion 1, so that we can really compare them in term of user experience.

Suggestion 1 doesn't imply any breaking but we have to find a way to generate the code in every language. And a lot of tests would be added, at least one for each new method.
Suggestion 2 involves big breaking but seems easy to code: just an Update object to add.
Suggestion 3 is the easiest one to code, and does not imply any breaking, but problem with the Go language! A lot of tests would be needed too.

=> The goal is to find what is the more user-friendly for our users, not only what we would like to code because it's easier 😇

@curquiza curquiza added integration guides need vote When several solutions are suggested labels Apr 28, 2020
@eskombro
Copy link
Member

eskombro commented Apr 28, 2020

We can pass an optional parameter, set by default to false, if user want to make it syncronous. Someting like turning

def add_documents(self, documents, primary_key=None):
...

into

def add_documents(self, documents, primary_key=None, wait_for_update=False):
...

The only problem is that we will still struggle with the Go SDK (and any other language that don't accpet optional params)

@curquiza
Copy link
Member Author

I added your suggestion to the list!

@bidoubiwa
Copy link
Contributor

bidoubiwa commented Apr 29, 2020

Suggestion 3

Because it still needs the interval and the timeout options it becomes a long list of function parameters.

def add_documents(self, documents, primary_key=None, wait_for_update=False, interval = 500, timeout= 2000):

It could also look like this:

def add_documents(self, documents, primary_key=None, wait_for_update=False, updateOptions = {
 interval = 500, 
 timeout= 2000
}):

( see syntax in every language)

Suggestion 2

I have look into function chaining in PHP:
https://www.unleashed-technologies.com/blog/2009/12/19/method-chaining-php

It is not used for our use case

Method chaining is best implemented on functions/methods that modify their parent object. They usually set a variable or perform some action without returning a result. Here are some examples of good candidates for chaining:

Which seems to be the case also for python. So I don't like about this option that requires a complete code restructure and does not seem to be a good practice in our case

Suggestion 1.

I tried to find a good naming for suggestion 1, those are my ideas.

CamelCase version

addDocumentsWaitUntilProcessed
addDocumentsAwaitProcessing
addDocumentsToProcessed
addDocumentsWaitUntilDone
addDocumentsWait
addDocumentsProcessed

snake case version

add_documents_wait_until_processed
addDocuments_await_processing
addDocuments_to_processed
addDocuments_wait_until_done
addDocuments_wait
addDocuments_processed

This function would still have its default parameters and added to that an object with the waiting options. Or just different parameter depending of the language

let status = addDocumentsWaitUntilDone(docs, {
  timeoutMs: 20000, // default 2000
  interval: 500 // default 500
})

Making these variables optionnal

My favorite

I think the most user-friendly is the suggestion 1. Because it does not require to struggle with additional parameters. It keeps your code more clean and by the name of the function you know what is going to happen and be returned.
You don't have to read the parameters to understand.

Mostly also, because in the case of suggestion 3, you could have a completely different return.
One will give you an updateId, the other a whole explanatory object of your processed action. Which is for me to big of a change in the possible return to be only one function.

  • return in case of wait_for_update=false:
{
 "updated":1
}
  • return in case of wait_for_update=true:
{
  "status": "processed",
  "updateId": 1,
  "type": {
    "name": "DocumentsAddition",
    "number": 4
  },
  "duration": 0.076980613,
  "enqueuedAt": "2019-12-07T21:16:09.623944Z",
  "processedAt": "2019-12-07T21:16:09.703509Z"
}

I also have difficulty imaging how we are going to document those function

def add_documents(self, documents, primary_key=None, wait_for_update=False, interval = 500, timeout= 2000):

return an object containing an updateId. This will help you track your process. If wait_for_update is set to true, you will instead recieve the update information once your process is not enqueued anymore.

Whereas if we go for suggestion 1

def add_documents(self, documents):

Add document to meilisearch.

return an object containing an updateId. This will help you track your process.

def add_documents_wait_until_done(self, documents, waitOptions):

Add document to MeiliSearch and wait until MeiliSearch has processed the action.

return an object containing information about your action status (Processed, or failed status and other useful information)

This is just an example without thought in phrasing but you get the general idea.

So yes Suggestion 1 is more user friendly and will make for a better documentation.

Integrating the solution

It is not as duplicating as I thought it would be.
This does not chock me. But maybe you have a better idea?

async function addDocumentsWaitUntilDone(docs, waitOptions) {
  const updateId = await addDocuments(docs)
  return waitForPendingUpdate(updateId, waitOptions.timeouts, waitOptions.interval)
}
Renaming the function ?

waitForEnqueudUpdate
Just a suggestion maybe we don't want to change the function name because that would be an unnecessary breaking. I suggest this since enqueued is the word used in meilisearch. But again, not needed if you don't feel like it

@curquiza curquiza added need discussion Need discussion to make a decision and removed integration guides need vote When several solutions are suggested labels Apr 15, 2021
@curquiza
Copy link
Member Author

Done in every SDK!!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
need discussion Need discussion to make a decision
Projects
None yet
Development

No branches or pull requests

3 participants