Skip to content

use Java CompletionStage instead of Scala Future in pekko-persistence Java APIs#2092

Merged
pjfanning merged 14 commits intoapache:mainfrom
pjfanning:persist-future
Aug 25, 2025
Merged

use Java CompletionStage instead of Scala Future in pekko-persistence Java APIs#2092
pjfanning merged 14 commits intoapache:mainfrom
pjfanning:persist-future

Conversation

@pjfanning
Copy link
Copy Markdown
Member

@pjfanning pjfanning commented Aug 25, 2025

raboof and others added 3 commits August 25, 2025 16:53
Sketching out apache#1417 - incomplete and notably not bothering
with binary compatibility yet, just to illustrate the idea.
@pjfanning pjfanning marked this pull request as draft August 25, 2025 16:01
@pjfanning pjfanning added this to the 2.0.0-M1 milestone Aug 25, 2025
persistenceId: String,
criteria: SnapshotSelectionCriteria): Future[Option[SelectedSnapshot]] =
doLoadAsync(persistenceId, criteria).map(option)
doLoadAsync(persistenceId, criteria).asScala.map(option)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

map(option)(org.apache.pekko.dispatch.ExecutionContexts.parasitic)

@pjfanning pjfanning marked this pull request as ready for review August 25, 2025 18:35
@mdedetrich
Copy link
Copy Markdown
Contributor

@pjfanning Regarding parasitic, for future reference whenever you do a map operation on Scala's Future, always pass in the explicit ExecutionContexts.parasitic. If you don't do this, the block inside of the map has to go into the implicit ExecutionContext (usually some kind of fork join pool), and that context switch is unnecessary overhead for what typically happens inside of a map.

Context switch is fine (and needed) for long running async operations, but those are typically composed with flatMap (since they would also be running in some kind of Future). When it comes to long running blocking that aren't inside of a Future, you would use the blocking/specific thread based dispatcher instead

@pjfanning
Copy link
Copy Markdown
Member Author

@He-Pin I made a couple of changes after your review was made including fixing an issue. Do you still think it is ok to merge as is?

Copy link
Copy Markdown
Member

@He-Pin He-Pin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm

@pjfanning pjfanning merged commit 5a8e161 into apache:main Aug 25, 2025
9 checks passed
@pjfanning pjfanning deleted the persist-future branch August 25, 2025 22:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants