-
Notifications
You must be signed in to change notification settings - Fork 6
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
feat: make it possible to filter message by origin #223
base: main
Are you sure you want to change the base?
Conversation
d80a8b4
to
16457cb
Compare
akka-javasdk/src/main/java/akka/javasdk/consumer/MessageContext.java
Outdated
Show resolved
Hide resolved
akka-javasdk/src/main/java/akka/javasdk/consumer/MessageContext.java
Outdated
Show resolved
Hide resolved
akka-javasdk/src/main/java/akka/javasdk/consumer/MessageContext.java
Outdated
Show resolved
Hide resolved
akka-javasdk/src/main/java/akka/javasdk/consumer/MessageContext.java
Outdated
Show resolved
Hide resolved
akka-javasdk/src/main/scala/akka/javasdk/impl/consumer/ConsumerImpl.scala
Outdated
Show resolved
Hide resolved
akka-javasdk/src/main/scala/akka/javasdk/impl/consumer/ConsumerImpl.scala
Outdated
Show resolved
Hide resolved
16457cb
to
f64be05
Compare
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.
Some updates on this PR.
- consumer and views receive
originRegion
in context - all components can have access to
selfRegion
viaContext
Some other ideas that we can include here or in follow-up. We might want to add API to set the originRegion in the Eventing Testkit.
/** | ||
* Returns the region where this instance is running. | ||
*/ | ||
String selfRegion(); |
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.
Context is ubiquitous. All contexts inherit from it. By adding the selfRegion
here we make it possible to always check 'where' we are.
This includes component creation contexts, command contexts, http request context and consumer messages (both consumers and views).
|
||
import java.util.Optional; | ||
|
||
public interface OriginAwareContext extends Context { |
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.
MessageContext (Consumer) and UpdateContext (Views) now extends OriginAwareContext
which gives access to the originRegion
and method hasLocalOrigin
.
* | ||
* @return the region where a message was first created. When not applicable, it returns an empty Optional. | ||
*/ | ||
Optional<String> originRegion(); |
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 mentioned this on the runtime side, but maybe I should do it here. In my opinion, it feels more natural to get the originRegion information from the metadata. We can add a helper method extract it, similar to subject
, time
, etc., I guess there was a reason to make it this way, what was that?
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.
hasLocalOrigin
is nice, but if that is the only reason, I think users can create such a method themselves if they need 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.
I think the implementation with meta will also be much simpler because we are passing this information through all the layers from the runtime to the sdk.
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 why I didn't add it to the metadata is because we used to also have JWT and ACL there and we stepped away from it. JWT and ACL is now only present on the endpoints request, which makes more sense.
Also, metadata is available in places where it doesn't make sense to have the originRegion
, for example in commands. We should have it only when consuming messages.
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.
About hasLocalOrigin
, the method impl is quite simple and indeed anyone can implement this logic, but there is some hidden knowledge in there that otherwise we will need to document.
- empty means we are consuming from a broker or service-to-service, so not local
(we will have to document this) - for non-empty origins, it needs to match selfRegion, so it depends
(this is easy and kind of obvious) - in dev-mode, both will be "" and therefore local by definition
(this is also hidden logic that this method is encapsulating)
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.
Also, metadata is available in places where it doesn't make sense to have the originRegion, for example in commands. We should have it only when consuming messages.
Good point, but that implies that we have a leaking abstraction because I can run metadata.asCloudEvent
in my command handler, which doesn't make any sense. Maybe we should have a separate CommandMetadata
, Message/EventMetadata
?
f64be05
to
7158c73
Compare
in docs we have a todo, related to this:
|
Sure, we will need to document this. |
7158c73
to
c6bd4a8
Compare
Draft since depending on https://github.com/lightbend/kalix-runtime/pull/3488