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
Distributed Data API for Akka Typed #23647
Conversation
} | ||
type GetSuccess[A <: ReplicatedData] = dd.Replicator.GetSuccess[A] | ||
type NotFound[A <: ReplicatedData] = dd.Replicator.NotFound[A] | ||
type GetFailure[A <: ReplicatedData] = dd.Replicator.GetFailure[A] |
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.
My thinking is that we could use the same messages as untyped.Replicator for response messages, but we have to use different for the requests, since we have to map the sender
.
Replies can then be sent directly back to the replyTo
actor.
Test PASSed. |
def key: Key[A] | ||
} | ||
|
||
final case class Get[A <: ReplicatedData](key: Key[A], consistency: ReadConsistency, request: Option[Any] = None)(val replyTo: ActorRef[GetResponse[A]]) |
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.
the reason I placed replyTo
in a second parameter list was that then it's nice with ask
, but on the other hand ask
is not the primary way you would interact with the Replicator so perhaps that is not worth it. It becomes rather ugly for the Update
message that already has one extra param list.
one could always use ref => Get(...)
for ask
javadsl is slightly more complicated, since it has to map the response types also (I think it would be a mess to have some types defined in typed.Replicator and some in untyped.Replicator) |
Test PASSed. |
}.recover { | ||
case _ ⇒ JReplicator.GetFailure(cmd.key, cmd.request) | ||
} | ||
reply.foreach { cmd.replyTo ! _ } |
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.
I was thinking about if we could have another spawnAdapter
that would just map the message and send to another target than self. The problem with that is the lifecycle though, so I stashed that idea for now.
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.
Mostly nitpicking, looking good.
*/ | ||
def apply(config: Config): dd.ReplicatorSettings = | ||
dd.ReplicatorSettings(config) | ||
} |
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.
create
rather than apply
for Java?
Update.modifyWithInitial(initial, data ⇒ modify.apply(data))) | ||
|
||
/** | ||
* Java API: Modify value of local `Replicator` and replicate with given `writeConsistency`. |
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.
Isn't everything here Java API?
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.
👍
* Send this message to the local `Replicator` to update a data value for the | ||
* given `key`. The `Replicator` will reply with one of the [[UpdateResponse]] messages. | ||
* | ||
* Note that the [[Replicator.Update$ companion]] object provides `apply` functions for convenient |
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.
should "companion" really go in the type-link-whatchamacallit?
|
||
trait Command[A <: ReplicatedData] extends scaladsl.Replicator.Command[A] { | ||
def key: Key[A] | ||
} |
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.
@DoNotInherit
, Not intended for user extension.
and sealed?
|
||
} | ||
|
||
sealed abstract class UpdateResponse[A <: ReplicatedData] extends NoSerializationVerificationNeeded { |
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.
@DoNotInherit
object Replicator { | ||
|
||
def behavior(settings: ReplicatorSettings): Behavior[Command[_]] = | ||
ReplicatorBehavior.behavior(settings) |
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.
A bit vague but: This compared to the much more locked down API versions for Singleton/Sharding, does it make more sense to give users access to running a replicator wherever they want in their tree? (and counter - should the Singleton/Sharding ones be more like this returning a behaviour and leave more options for the user?)
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.
There will also be an extension as a convenience, same as untyped ddata. It must still be possible to start your own Replicator instances because it should be possible to run several with different settings (e.g. performance reasons).
Comparing to singleton. In the new api spawn
is starting both manager and proxy so that makes sense. We could consider if spawnManager
and spawnProxy
should instead only be Behavior
factories and user performs the ctx.spawn
. More flexible and we said that those are for power user api anyway. Drawback might be that it's easier to get actor paths wrong.
Sharding must be an extension.
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.
I think that is good, power-user returns behaviour and it's up to user to get it right, I'll change the singleton API
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.
ok
2fdaaae
to
6d78e30
Compare
Refs #23664 |
I have completed the remaining things and this is ready for final review. |
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!
No description provided.