-
Notifications
You must be signed in to change notification settings - Fork 10
feat: Replication filters #292
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
base: main
Are you sure you want to change the base?
Conversation
| return ReplicationFilterImpl.empty().removeRegion(region); | ||
| } | ||
|
|
||
| ReplicationFilter addRegion(String region); |
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.
would have been nice if the static method could have been named the same, but that's not possible with Java
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 usage of it will be something like:
ReplicationFilter
.includeRegion("region-1")
.addRegion("region-2")and that is odd.
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 we can solve it with an intermediate interface called BaseReplicationFilter.
The base defines the instance methods.
The static methods in ReplicationFilter should return BaseReplicationFilter.
The ReplicationFilterImpl should implement BaseReplicationFilter.
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.
thanks, changed to something like that in ef93a53
|
|
||
| import akka.javasdk.impl.effect.ReplicationFilterImpl; | ||
|
|
||
| public interface ReplicationFilter { |
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 want to have this rather open for future additions of other types of filters, than regions.
johanandren
left a comment
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.
SDK side looking good so far
| keyValue = false) | ||
| keyValue = false, | ||
| replicationFilterEnabled = true | ||
| ) // FIXME how shall we enable it? Can we solve it without enable flag? |
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.
Enabling in the sense that a user opts in, or in the sense that the SDK supports it?
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.
"user opts in". By default we replicate to all regions, until the user defines a filter with regions. However, on the runtime side the filter mechanism is added at startup.
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.
Yeah, I got that part about replicate everywhere by default, but this toggle, is this to say SDK supports it and then user opts in by defining filters, or this should be true/false based on user choice in entity definition/annotation something?
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 original thought was that the user would have to enable this for a specific entity. Could be an annotation or a method override. However, that is rather clunky and it would be better to enable by setting the replication filter in the effect, but that happens later. Not sure yet if we can get rid of this flag.
| } | ||
| for (String region : item.removeRegions()) { | ||
| filter = filter.removeRegion(region); | ||
| } |
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.
If we expect a lot of non-single add/remove (not sure we do), it would be nicer with prepared methods taking Collection<String>
|
|
||
| if (item.productId().isEmpty()) { | ||
| return effects() | ||
| .replicationFilter(filter) |
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.
Mismatch between persist which is imperative and replicationFilter, would be better if it was also imperative, even if longer, something like updateReplicationFilter or setReplicationFilter
2d7682d to
8e42c93
Compare
8e42c93 to
e8a7720
Compare
|
Refreshed, ready for review |
| ---- | ||
|
|
||
| If you start with an entity with such self region filter in `gcp-us-east1`, and then later receive a command (not read-only) for this entity instance in another `aws-us-east-2`, it will automatically synchronize all events in from `gcp-us-east1` before handling the command in `gcp-us-east1`. Such command will also automatically include `aws-us-east-2` to the replication filter and events will be replicated to both `gcp-us-east1` and `aws-us-east-2`. You can remove a region from the replication with `ReplicationFilter.excludeRegion`. | ||
|
|
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 how we can handle replication filter for the agent session memory. Perhaps we can always make the session memory self region only, as described above? If the session is used from another region it will sync and automatically add that region in the filter.
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.
Wouldn't that defeat the idea that the user should decide where to replicate?
I get that once the session memory is used on a new region, we need to start to replicate. But will users be able to block it?
And there is another issue. It will be all session memory events that will start to replicate because it's all the same entity.
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 want to start with something simple, and therefore included the self region filter in the session memory: 4332bda
Otherwise we would have to add filters to the MemoryProvider so that it can be defined in the agent, and passed on to the SessionMemoryEntity. Let's see if there is demand for that.
It will be all session memory events that will start to replicate because it's all the same entity
yeah, I don't see a problem with that. it's the session. We could add some kind of region filtering to the MemoryFilter, but let's wait for real demand for such thing
octonato
left a comment
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.
Looking good
Left some comments for now. I will continue reviewing.
| } | ||
|
|
||
| override def updateReplicationFilter(filter: ReplicationFilter): OnSuccessBuilder[S] = { | ||
| _replicationFilter = filter.asInstanceOf[ReplicationFilterImpl] |
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.
It seems that the only reason to cast here is because later we want to call toSpi on it.
Could the interface have accessors for toIncludeRegions and toExcludeRegions instead?
We can then transform to SPI using some static method receiving the interface time.
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, changed in 9e3ff88
| * INTERNAL API | ||
| */ | ||
| @InternalApi | ||
| private[javasdk] final case class ReplicationFilterImpl(addRegions: Set[String], removeRegions: Set[String]) |
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.
nitpicking, but I think the names here should be includedRegions and excludedRegions
| @Target(ElementType.TYPE) | ||
| @Retention(RetentionPolicy.RUNTIME) | ||
| @Documented | ||
| public @interface EnableReplicationFilter {} |
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 don't get why we need the annotation.
Maybe I should go back to the runtime PR to understand it, but if you can provide a short explanation, that will help.
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, I went back to the runtime PR and now it's clear why.
We need to configure the consumer and we need this info in the descriptor. Makes sense.
| return ReplicationFilterImpl.empty().removeRegion(region); | ||
| } | ||
|
|
||
| ReplicationFilter addRegion(String region); |
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 usage of it will be something like:
ReplicationFilter
.includeRegion("region-1")
.addRegion("region-2")and that is odd.
| return ReplicationFilterImpl.empty().removeRegion(region); | ||
| } | ||
|
|
||
| ReplicationFilter addRegion(String region); |
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 we can solve it with an intermediate interface called BaseReplicationFilter.
The base defines the instance methods.
The static methods in ReplicationFilter should return BaseReplicationFilter.
The ReplicationFilterImpl should implement BaseReplicationFilter.
d6989bf to
ef93a53
Compare
| * to include one region and then later make another update to include a different region from | ||
| * the same entity, both regions are included. | ||
| * | ||
| * <p>After enabling replication filter with the {@code @EnableReplicationFilter} annotation, |
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 would rephrase this paragraph or remove it. Maybe we should mention the annotation at the beginning that it's required when using this method?
| * to include one region and then later make another update to include a different region from the | ||
| * same entity, both regions are included. | ||
| * | ||
| * <p>After enabling replication filter with the {@code @EnableReplicationFilter} annotation, the |
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.
Here, this paragraph duplicates the information.
| <1> Enable the replication filter feature by adding the `@EnableReplicationFilter` annotation. | ||
| <2> Define the replication filter with the `updateReplicationFilter` effect. | ||
|
|
||
| After enabling the replication filter the entity is still replicated to all regions until the regions are defined with the `updateReplicationFilter` effect. This effect can be combined with persisting events or used without additional events. The filter can only be updated from the entity's primary region. With the `request-region` primary selection strategy, updating the filter from a non-primary region will cause that region to become the new primary. The filter is durable for the specific entity instance and can be changed without deploying a new version. |
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.
| After enabling the replication filter the entity is still replicated to all regions until the regions are defined with the `updateReplicationFilter` effect. This effect can be combined with persisting events or used without additional events. The filter can only be updated from the entity's primary region. With the `request-region` primary selection strategy, updating the filter from a non-primary region will cause that region to become the new primary. The filter is durable for the specific entity instance and can be changed without deploying a new version. | |
| After enabling the replication filter the entity is still replicated to all regions until specific regions are defined with the `updateReplicationFilter` effect. This effect can be combined with persisting events or used without additional events. The filter can only be updated from the entity's primary region. With the `request-region` primary selection strategy, updating the filter from a non-primary region will cause that region to become the new primary. The filter is durable for the specific entity instance and can be changed without deploying a new version. |
| <1> Enable the replication filter feature by adding the `@EnableReplicationFilter` annotation. | ||
| <2> Define the replication filter with the `updateReplicationFilter` effect. | ||
|
|
||
| After enabling the replication filter the entity is still replicated to all regions until the regions are defined with the `updateReplicationFilter` effect. This effect can be combined with persisting events or used without additional events. The filter can only be updated from the entity's primary region. With the `request-region` primary selection strategy, updating the filter from a non-primary region will cause that region to become the new primary. The filter is durable for the specific entity instance and can be changed without deploying a new version. |
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.
This effect can be combined with persisting events or used without additional events. -> Maybe add some additional clarification on when to use which, sth like see below for replication configuration with events persistence use case.
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 tried to incorporate your feedback in bfa6156
|
|
||
| public Effect<Done> replicateTo(String region) { | ||
| return effects() | ||
| .updateReplicationFilter(ReplicationFilter.includeRegion(region)) // <2> |
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.
Since this configuration is additive, are we planning to somehow provide the information about the current replication filter configuration? Either via ESE state or backoffice or maybe both.
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.
Both specific replication and the more static filtering enabled flag would be nice to show in backoffice IMO
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.
created an issue for backoffice https://github.com/lightbend/akka-runtime/issues/4449
I have also thought about the need for exposing current filter in the entity context, but haven't seen a clear use case for it
johanandren
left a comment
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 some small docs feedback.
| * | ||
| * <p>The session memory has the multi-region replication filter enabled to only include the local | ||
| * region when using `request-region` primary selection. When accessed from another region the | ||
| * filter will be expanded to include the other region too. |
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 should probably mention this in the reference docs too
* Reflect isReplicationFilterEnabled * ReplicationFilterImpl since that would be same for kve and workflow * docs, JavaDoc * replication filter in SessionMemoryEntity * ReplicationFilter.Builder to be able to use the same method names * note about excluded region * incorporate feedback
bfa6156 to
68cf17d
Compare
No description provided.