Skip to content
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

Rewrite Source.actorRef as GraphStage #25324

Closed
patriknw opened this Issue Jul 9, 2018 · 11 comments

Comments

Projects
None yet
5 participants
@patriknw
Copy link
Member

commented Jul 9, 2018

Also consider the issue with PoisonPill that was discovered in #25286

@lasekio

This comment has been minimized.

Copy link

commented Jul 9, 2018

It seems to be very similar to Source.queue unless support for backpressure, because of tell api I guess. Can we build on top of it somehow or you want it to be completely separated implementation?

@patriknw

This comment has been minimized.

Copy link
Member Author

commented Jul 9, 2018

I think it’s worth to have it separate. It can probably make use of the ActorRef of the stage (if that can be returned as materialized value).

@lasekio

This comment has been minimized.

Copy link

commented Jul 9, 2018

I cant use stage actor in GraphStageWithMaterializedValue, because it is not initialized yet, during logic construction when we have to provide materialized value.

@nachinius

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2018

@patriknw I took a look and found the following.

Current Source.actorRef uses a SourceModule instead of a GraphStageWithMaterializedValue.
There is a reason for it. With SourceModule, there it has access to a MaterializationContext before it's being materialized, whereas with GraphStageWithMaterializedValue there is access to a similar context during the materialization phase itself.

I see basically two way so far,
a) altering the API GraphStageWithMaterializedValue.createLogicAndMaterializedValue, so it also receives somehow a materialize in which it can create the ActorRef
b) to materialize a Future[ActorRef] instead of a ActorRef. In this case, the actorRef is created in the GraphStageLogic.preStart()

By the way, what's the need for this refactoring?

@patriknw

This comment has been minimized.

Copy link
Member Author

commented Aug 15, 2018

By the way, what's the need for this refactoring?

Because it's implemented with the deprecated akka.stream.actor.ActorPublisher that we eventually (2.6.0) want to remove.

Seems related to #24372 where we also would prefer non-Future materialized value. We might have to come up with something new to be able to create such stages. Not sure what it would be right now.

@nachinius

This comment has been minimized.

Copy link
Contributor

commented Aug 15, 2018

Yes, it's related. I proposed a solution in #24372.

@nachinius

This comment has been minimized.

Copy link
Contributor

commented Aug 16, 2018

Since the akka.stream.actor.ActorPublisher is being removed soon, a lot of its functionality should be ported/implemented somewhere inside the Source.ActorRef, right? This actorRef is a stream publisher, and this is basically the job of ActorPublisher.
Thus, would it make sense to keep of the ActorPublisher (or a simplified version) as an internal api?

@ktoso

This comment has been minimized.

Copy link
Member

commented Aug 16, 2018

Thus, would it make sense to keep of the ActorPublisher (or a simplified version) as an internal api?

I'd rather say no, since everything it can do a GraphStage can do and saver + fuseable.

@nachinius

This comment has been minimized.

Copy link
Contributor

commented Aug 17, 2018

How do you connect the GraphStage with the ActorRef?

Every time there is demand, the GraphStage must check that ActorRef for messages, obtaining them if any, and pushing it out. I don't see how to do this, without implementing in the Actor behind the ActorRef some type of Subscriber. It may be possible to leverage a StageActor (not sure if it's intended for other things that are not watching), or some very advanced use of SourceRef. Thoughts?

@nvollmar

This comment has been minimized.

Copy link
Contributor

commented Dec 6, 2018

I prepared a sample implementation for graph actor ref source based on a distributed pubsub source I've implemented once: #26054

To exemplify the two issues already mention:

  • stage actor ignores PoisonPill => either a dedicated message is required or stage actor's behaviour must be adapted (mapping poison pill to an internal message to be matched for example)
  • stageActor can only be created in preStart which prevents us from returning the actor ref directly
@nvollmar

This comment has been minimized.

Copy link
Contributor

commented Mar 8, 2019

@patriknw I've updated PR #26054 with a possible implementation for it (with internal access to materializer)
It still needs a little additional work to complete (side-effects in other tests, PoisonPill backwards compatible handling etc) but might be a way to replace the deprecated source.

nvollmar pushed a commit to nvollmar/akka that referenced this issue Mar 8, 2019

nvollmar pushed a commit to nvollmar/akka that referenced this issue Mar 8, 2019

nvollmar pushed a commit to nvollmar/akka that referenced this issue Mar 8, 2019

nvollmar pushed a commit to nvollmar/akka that referenced this issue Mar 9, 2019

nvollmar pushed a commit to nvollmar/akka that referenced this issue Mar 18, 2019

nvollmar pushed a commit to nvollmar/akka that referenced this issue Mar 18, 2019

nvollmar pushed a commit to nvollmar/akka that referenced this issue Mar 18, 2019

nvollmar pushed a commit to nvollmar/akka that referenced this issue Mar 18, 2019

nvollmar pushed a commit to nvollmar/akka that referenced this issue Mar 18, 2019

nvollmar pushed a commit to nvollmar/akka that referenced this issue Mar 21, 2019

nvollmar pushed a commit to nvollmar/akka that referenced this issue Mar 21, 2019

nvollmar pushed a commit to nvollmar/akka that referenced this issue Mar 21, 2019

nvollmar pushed a commit to nvollmar/akka that referenced this issue Mar 21, 2019

nvollmar pushed a commit to nvollmar/akka that referenced this issue Mar 22, 2019

nvollmar pushed a commit to nvollmar/akka that referenced this issue Mar 25, 2019

nvollmar pushed a commit to nvollmar/akka that referenced this issue Mar 25, 2019

nvollmar pushed a commit to nvollmar/akka that referenced this issue Mar 25, 2019

nvollmar pushed a commit to nvollmar/akka that referenced this issue Mar 26, 2019

nvollmar pushed a commit to nvollmar/akka that referenced this issue Mar 26, 2019

nvollmar pushed a commit to nvollmar/akka that referenced this issue Mar 26, 2019

nvollmar pushed a commit to nvollmar/akka that referenced this issue Mar 26, 2019

nvollmar pushed a commit to nvollmar/akka that referenced this issue Mar 26, 2019

nvollmar pushed a commit to nvollmar/akka that referenced this issue Mar 26, 2019

nvollmar pushed a commit to nvollmar/akka that referenced this issue Mar 26, 2019

patriknw added a commit that referenced this issue Mar 27, 2019

GrapheStage implementation for actorRef source (#25324) (#26054)
* Adds internal access to materializer before initialization (#25324)

* Implements new actorRef source based on graph stage  (#25324)

* Removes obsolete actorRef source (#25324)

* Improves backwards compatibility with old implementation (#25324)

* Removes dedicated new subclass for materializer access again  (#25324)

* Improves implementation (#25324)

* Finalizes implementation (#25324)

* Small improvements to API and documentation (#25324)

* Completion strategy as a replacement for poison pill (#25324)

* Adding more tests and updating the documentation (#25324)

@patriknw patriknw added this to the 2.5.22 milestone Mar 27, 2019

@patriknw patriknw closed this Mar 27, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.