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

akka.PublishSubscribe - new setting and small fixes #4649

Merged
merged 3 commits into from
Dec 8, 2020

Conversation

zbynek001
Copy link
Contributor

new setting send-to-dead-letters-when-no-subscribers, some collections in messages switched to immutable, bugfix in PerGroupingBuffer

@zbynek001 zbynek001 changed the title akka.PublishSubscribe - new setting and small fixes [WIP] akka.PublishSubscribe - new setting and small fixes Dec 3, 2020

Mediator.Tell(new Tools.PublishSubscribe.Internal.Status(deltaBuckets2.ToDictionary(b => b.Owner, b => b.Version), isReplyToStatus: false));
Mediator.Tell(new Tools.PublishSubscribe.Internal.Status(versions1.SetItems(deltaBuckets2.ToImmutableDictionary(b => b.Owner, b => b.Version)), isReplyToStatus: false));
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this test had random behavior, depending on the order in dictionary. In some cases it lost the version info from the previous runs and returned data multiple times

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice fix - this one has definitely been racy in the past.

_totalBufferSize += 1;
}

action();
else
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is the actual fix

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice catch

@zbynek001 zbynek001 changed the title [WIP] akka.PublishSubscribe - new setting and small fixes akka.PublishSubscribe - new setting and small fixes Dec 3, 2020
@Aaronontheweb Aaronontheweb added this to the 1.4.13 milestone Dec 8, 2020
Copy link
Member

@Aaronontheweb Aaronontheweb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but I need to send in a small PR to avoid the breaking constructor change


Mediator.Tell(new Tools.PublishSubscribe.Internal.Status(deltaBuckets2.ToDictionary(b => b.Owner, b => b.Version), isReplyToStatus: false));
Mediator.Tell(new Tools.PublishSubscribe.Internal.Status(versions1.SetItems(deltaBuckets2.ToImmutableDictionary(b => b.Owner, b => b.Version)), isReplyToStatus: false));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice fix - this one has definitely been racy in the past.

@@ -131,13 +131,13 @@ public static Props Props(DistributedPubSubSettings settings)
/// <summary>
/// TBD
/// </summary>
public IDictionary<Address, long> OwnVersions
public IImmutableDictionary<Address, long> OwnVersions
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Technically a breaking API change but not really - no one uses the actor's API directly.

/// <exception cref="ArgumentException">TBD</exception>
public DistributedPubSubSettings(string role, RoutingLogic routingLogic, TimeSpan gossipInterval, TimeSpan removedTimeToLive, int maxDeltaElements)
public DistributedPubSubSettings(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Probably need to add a constructor overload here to avoid a breaking change that will have real binary compatibility impact... I'll do that in a subsequent PR

_totalBufferSize += 1;
}

action();
else
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nice catch

# The id of the dispatcher to use for DistributedPubSubMediator actors.

# When a message is published to a topic with no subscribers send it to the dead letters.
send-to-dead-letters-when-no-subscribers = on
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@Aaronontheweb Aaronontheweb merged commit c8add77 into akkadotnet:dev Dec 8, 2020
@Aaronontheweb Aaronontheweb mentioned this pull request Dec 8, 2020
This was referenced Dec 16, 2020
@zbynek001 zbynek001 deleted the publish-subscribe-fix branch July 16, 2021 12:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants