-
Notifications
You must be signed in to change notification settings - Fork 1k
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
Have ShardingAdapter
recursively call the underlying IMessageExtractor
#7474
base: dev
Are you sure you want to change the base?
Have ShardingAdapter
recursively call the underlying IMessageExtractor
#7474
Conversation
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.
Detailed my changes
@@ -76,7 +76,7 @@ public ExtractorAdapter(IMessageExtractor underlying) | |||
{ | |||
return message switch | |||
{ | |||
ShardingEnvelope se => se.Message, | |||
ShardingEnvelope se => _underlying.EntityMessage(se.Message), |
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 call the underlying message extractor's EntityMessage
method on the payload of the ShardingEnvelope
, just in case that also needs unwrapping. This is needed in order to make sure that messages still work properly when using tools like Akka.Cluster.Sharding.Delivery, which uses the ShardingEnvelope
heavily.
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 we do a recursive search here instead, just to make doubly sure?
something in the line of
private object UnwrapShardingEnvelope(object message)
{
object unwrapped = message;
while(unwrapped is IWrappedMessage)
{
if(unwrapped is ShardingEnvelope)
return unwrapped;
unwrapped = unwrapped.Message;
}
return message;
}
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.
No because that's introducing side-effects the user is ultimately responsible for - we only care about the ShardingEnvelope
specifically
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.
Something to think about
@@ -76,7 +76,7 @@ public ExtractorAdapter(IMessageExtractor underlying) | |||
{ | |||
return message switch | |||
{ | |||
ShardingEnvelope se => se.Message, | |||
ShardingEnvelope se => _underlying.EntityMessage(se.Message), |
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 we do a recursive search here instead, just to make doubly sure?
something in the line of
private object UnwrapShardingEnvelope(object message)
{
object unwrapped = message;
while(unwrapped is IWrappedMessage)
{
if(unwrapped is ShardingEnvelope)
return unwrapped;
unwrapped = unwrapped.Message;
}
return message;
}
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
Looks like this introduced a real test failure |
Changes
close #7470
Checklist
For significant changes, please ensure that the following have been completed (delete if not relevant):
ShardingEnvelope
contents inIMessageExtractor
#7470