Get result api

proposal

#1

We have a problem when we need to execute a task and listen for its result.
We solve it by adding a time.Sleep between listenResult and executeTask, eg:

The same pattern is used in the api package and also in a lot of simple application. We were also thinking to create an helper in the mesg-js lib to ease the use of this weird pattern.

Does it make sense to use the listenResult stream to get the result of only one execution? No.
Even more, what if we want to get the result of a past execution? We don’t have any api for it.

That’s why I’m suggesting to add a new very simple api:

service Core {
  // Get the result of a specific execution.
  // If the execution is still in progress, it waits until the execution finishes.
  rpc GetResult(GetResultRequest) returns (ResultData) {}
}

message GetResultRequest {
  string executionID = 1;             // The unique identifier of the execution.
}

@Anthony @krhubert @ilgooz what do you think?


Acknowledgement system on stream
#2

I understand the problem we want to solve here. However adding a new api should be the last option specially when we already have a similar one.

We shouldn’t be adding this new api to solve the need of adding sleep. Because if stream is not ready to serve results when call to ListenResult from client is returned, this is some kind of a bug that we need to solve.

On the other hand, getting results from past is a new feature. Do we really want it? Is there a use case for getting past execution results from execution id or even with other filters? If so, we should maybe consider adding the same for events as well.


#3

Agree with @ilgooz, the problem with the ListenResult should be solved and adding a new API will not change anything (especially an unary call that needs to wait for a result this would lead to many timeouts).

  • getResult API is interesting but I think a GetExecution would make a lot more sense
  • getExecution would return the execution object (event, the state of the execution and the result)
  • We could rename listenResult to listenExecution
  • listenExecution would listen for every change of state from an execution (with potential filters) and return an execution object
  • execute API should return the same execution object

For me this would make sense but this is new feature (that will be required) but that shouldn’t be a way to fix the listenResult (even if a polling on the getExecution would solve that)

Conclusion:

Not a fix for the listenResult
New interesting feature not about result but about executions (that are entries in the database)


#4

Why last option? There is nothing wrong with adding new api - we don’t participate in a contest for the lowest method count in api. Also similar api doesn’t mean the same.

Moreover, we don’t have very complicated api. Take a look at any popular services api - there are hundreds of entries.

But here is the point, there is nothing wrong with the ListenResult and there is no bug. The way we were thinking of this api was simply wrong and this is the pbm. ListenResult is like stream for results and it should be used that way (not to get a single result) - like in dev command. While doing execution and getting results the “natural” flow is:

  • execute the task
  • get the task results

What we do now is (with status ready “bug” fix):

  • generate uuid
  • Listen for task
  • wait for status ready
  • execute the task
  • read from channel results
  1. we lost time while waiting for status readiness (execution could be fired first as probably it will take longer)
  2. there are 5 steps compared to 2 just to “fit to api”
  3. we’re gonna execute lots of task in system services and we don’t want to lose time for waiting for stream (but this is in the future) (btw, we don’t want to lost time in pkgs/libs as well)
  4. we use listen (aka stream) results to get just one response. We can say that we listen for only one specific results, but if you start saying what we are doing then you realize that we want to execute and get results. We’re generating the uuid to listen for specific task and this is how we “bullshit” ourself that liste results api is all we need. In fact we should be getting the results by exeuction ID and not “magicaly uuid”

As you point out, the getExecution is diffrent api

We spoke about this but we want to keep current naming convention and do one thing at a time. We also speak about naming convention in general in the whole core but it’s diffrent topic.

It’s ok similar to listen results

So listen results is totally fine :slight_smile: and don’t need to be fixed.
I also think that we could add some kind of stream readiness (as @Nicolas did in POC), even we should, but the pbm with this is we join two “bugs” into one.

  • one bug is lack of stream readiness info (it’s could be implemented but usage of it is optional)
  • the other is wrong usage of listen results (like invalid thinking)

They both deserve to be fixed :smiley: but we can’t add the readiness info and pretend that the second bug was solved by doing so and this thread is about the second bug.


#5

The problem I see with a getResult api is when the execution didn’t finish yet.

  • If the call wait for the execution to finish, then the client could receive connection timeout error because the execution didn’t return any result fast enough.
    I don’t like this approach, a unary call should return “immediately”. Otherwise, it should be a stream.

  • If the call doesn’t wait, then the client have to do polling in order to get the result if the execution didn’t finish on time.
    The problem here is ResultData doesn’t contain any info about the status of the execution.
    That’s why it make sense to implement this api by returning all the Execution data, thus renaming the api to getExecution. This way, the client can actually know the state of the execution (and its result if it finished).
    Then we should also implement a stream listenExecution (and deprecate listenResult) that allows client to be notified of EVERY change of state of executions (and not just the completed and failed state with current listenResult stream).

So actually, getResult will not solve the problem is a nice way and we should implement:

  • getExecution(executionID) executionData
    Returns all execution data from the execution database based on a specific execution id.

  • listenExecution(filters) stream executionData
    Be notified of state change of matching executions.

  • deprecate listenResult in favor of listenExecution with a filter that match executions with status completed or failed.

  • even more, we could also deprecate listenTask api on the service side. it can be done by using listenExecution with a filter that match executions with status created and the right service id.
    Even better, the service could set the execution as in progress when it start to process it! (currently, the core set the execution as in progress when it send it to the service), but it requires to add another api updateExecution (that could actually also deprecate the current submitResult api!).

Yeah… we can improve our API a lot with an generic approach using Execution! :sunglasses:
Also, having Execution apis will make the sync of Execution across multiple Core way easier.


#6

I haven’t though about that, that’s a good point, I’m just afraid that it might be complicated for developers of services because they will always have to set the filter to status=created but definitely something to thing about as well as the updateExecution :slight_smile:

I really think we shouldn’t have any “result” in the api but only executions, now it’s weird because we have an executionID but there is no notion of execution in the API and result is just a subset of the execution.

Do we really need to deprecate other apis, we could just replace/rename them because we are still in 0.x.


#7

I don’t get the point here. The arguments are the same for getExecution/listenExecution:

  • if get execution dosen’t wait then client have to do polling
  • if execution dosne’t change it states, then we need to have some sort of timeout (for listening resuts it’s the same)
  • listen execution should notify about readiness

But of course, Execution API is what we should implement and relay on this. Because as @Anthony pointed out:

And about deprecation - let’s not bother with this, we’re in alpha stage and let’s use benefits of being in it :slight_smile:

:+1:


#8

Execution API is an another feature. For the issue with streams, I think it’s some kind of bug and we need ack system to solve it. Acknowledgement system on stream


#9

Agree with @ilgooz this one should be marked as solved when ack is done and a new proposal about the api using Execution instead of Result should be created


#10

Disagree, as I wrote, ack is a totally different issue it isn’t solution to this problem. Of course it will “fix the problem” but it’s rather workaround then solution. So in fact ack is what we should propose in new thread and here apply “real” solution


#11

Let’s close this topic for now and open a new one about implementing Execution APIs