-
Notifications
You must be signed in to change notification settings - Fork 357
-
Notifications
You must be signed in to change notification settings - Fork 357
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
Merge/clean up SAEA and ABJEA, and remove the promises-of-results, futures-of-handles, etc. #1218
Comments
This is unblocked and ready for fixing. |
More details that emerged over the past couple weeks: In the past, the Local and JES backends extended the ABJEA. When the Local and JES backends were merged into Standard, there was a lot of work in the Standard not to mess with the existing ABJEA. This turns out to have not been necessary. The only known extension of the ABJEA is now the Standard implementation, the Thus, one is now free to merge the SAEA and ABJEA, and remove the promises-of-results, futures-of-handles, etc. with a simplified actor/fsm. Similarly, while there is a Standard actor that extends the Also, when considering rewrites, it was also noted that the term "sync/async" in the SSEA/SAEA names were confusing. Possible alternatives were StandardFutureExecutionActor / StandardMessagingExecutionActor. Again this may be moot if the actors are merged into a single actor. |
@kshakir Can you give me an idea of the effort involved for this? |
I'd call this a "medium" effort for a developer. It's not a quick fix, while not as much as an effort as "implement CWL". A couple weeks of refactoring, a round of testing and team review, then another two weeks to polish it off. |
As a user developing a backend for Cromwell, I want the execution actor naming and system to be simple and concise, so that I can more easily work with Cromwell, and don't add unnecessary code.
|
Promise
between Backend Executor and Async
Closing this as it's a year and a half old @kshakir - if you'd like something to change in this regard, put together a tech doc of what you'd like to see happen. |
AsyncBackendJobExecutionActor (ABJEA) uses a
completionPromise: Promise
to tell its parent actor, the BackendJobExecutionActor (BJEA), when the async job is done.I doesn't seem obvious why we're passing around references to
scala.concurrent.Promise
s when akka has a perfectly good message system already.Worst case, if one needed two different states and didn't want the framework to use an FSM, the BJEA could
context.become(initializing)
, start the async job, thencontext.become(asyncRunning)
, and wait for the message from the ABJEA.The text was updated successfully, but these errors were encountered: