-
Notifications
You must be signed in to change notification settings - Fork 15
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
Streaming messages from ViperCoreServer #15
Comments
" ... or is it the case that the streaming starts only when the future returned by lookupJob is complete." Yes, this is the case. " ... if the current implementation of streamMessages actually streams messages as soon as they are available in the source queue, ..." Yes, this is the case as well. ;) I'll admit that this sounds contradictive. But there's a crux: handle_future is completed as soon as possible:
In summary: we indeed only start streaming once the handle_future is completed, but this is not a problem as the future is immediately completed. |
Addendum 1 In case you wonder why there's a future at all if it is immediately resolved: When communicating with actors the answers always come back as Future[Any]. Whatever we do, we have to wait back for that future before doing anything related to a handle. Addendum 2 This comment is wrong. viperserver/src/main/scala/viper/server/ViperCoreServer.scala Lines 69 to 73 in 2e3bbbd
Let me know if you agree and I'll correct the comment. |
Thanks, @WissenIstNacht , for clarifying this. I was indeed confused by the comments above the definition of Since |
Sure, I'll add the necessary comments.
No, at the moment the API does not support this. |
Thanks for adding the comments. I would like to propose the following new signature for the method def streamMessages(jid: Int, clientActor: ActorRef): Option[Future[Bool]] The return type should be Could you add this functionality, please? |
I would like to voice a few remarks/questions on this proposal: What exactly should the Boolean type express in you signature? When should the Future be completed? I think a Boolean should answer a question with yes or no. From your description I get the feeling that it answers two different questions, one with yes and one with no. Also, a state of (in)completion is exactly what an (in)completed Future expresses. Why then put the fact that a Future is (not) resolved into the Boolean type that is contained in that Future? Also, while the API does not provide this information, it doesn't mean the information does not exist. In fact, the flow of information was precisely set up in such a way that it does. A client who uses an actor to communicate with ViperCoreServer will get a stream of messages where the last message is a overall success/failure. This indicates all you're asking for: The result of the process, the fact that it's over and even whether or not a verification job exists (i.e., has been started) or not. I feel like what you proposed should be the return type of a method called getOverallVerificationResults and should not be the return type of streamMessages. Recall, the point of streamMessages was to attach an actor to ViperCoreServer and "continue" the stream/actor-based architecture. In my view, adding this signature is not in line with the API's design. |
Hi Valentin, These are valid concerns, so let me try to address them (the order is random).
I agree that one could use an actor to pattern match on Overall results and find out when the verification is completed. So you are right that, in principle, one could circumvent the issue. However, I think the API should provide a more streightforward way to get the information about when the verification job has ended. This information is needed for any client that uses streams, so let's provide it directly instead of asking the client to retrieve it on their own.
Actually, no: when the client is interested only in the overall results (and not the intermediate ones), then they would get the information about the end of this verification job for free. This is what happens in the scenario in which the client uses
There are four possible designs that come to mind. It seems that you don't disagree with the
|
Thanks for implementing this API change. |
I want to understand if the current implementation of
streamMessages
actually streams messages as soon as they are available in the source queue, or is it the case that the streaming starts only when the future returned bylookupJob
is complete.Concretely, here is the code snippet:
It seems that the materialization (i.e. the call to
runWith
) only happens after the future is completed. Shouldn't we first start the stream, then finalize it once the job is completed?See full code here:
viperserver/src/main/scala/viper/server/ViperCoreServer.scala
Line 338 in 2e3bbbd
The text was updated successfully, but these errors were encountered: