-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
Change typed whenTerminated sig to Future[Done], #25647 #26644
Conversation
Test PASSed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM with one little nit
override def getWhenTerminated: CompletionStage[Terminated] = FutureConverters.toJava(whenTerminated) | ||
private val terminationPromise = Promise[Done] | ||
override def terminate(): Unit = terminationPromise.trySuccess(Done) | ||
override def whenTerminated: Future[Done] = terminationPromise.future.map(_ => Done)(sameThreadExecutionContext) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No need to map if it is already a Future[Done]
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doh, thx
This change ignore the terminated passed from untyped and map it into Done, with some minor changes for testing termination. termiate() returns Unit to not bias it towards the Scala API, completion can be observed with whenTerminated or getWhenTerminated
0200d7f
to
7cc6266
Compare
Test PASSed. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Rebased #25651
Also changed
terminate()
to returnUnit
to not bias it towards the Scala API, completion can be observed with whenTerminated or getWhenTerminatedRefs #25647