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

Allow AtLeastOnceDelivery parameters to be set from deriving classes (as intended) #3810

Merged
merged 11 commits into from Jul 20, 2019

Conversation

@ondrejpialek
Copy link
Contributor

commented May 25, 2019

Currently all AtLeastOnceDelivery parameters are read from settings passed via ctor when the semantic is created. The documentation says that properties that expose these parameters may be overwritten in derived classes to provide different values. This however does not work. The semantic never reads from the derived actors themselves (it has no reference to the owning actor to even read the properties).

This PR makes the owning actors' properties non-virtual and write through, effectively encapsulating the ones of the semantic. The semantic is additionally updated to handle changing values even as the actor runs (so they can be modified even outside the actor's ctor and the new values will be picked up by the semantic).

The values from settings are still the default.

I also fixed the redelivery interval being fired at double the intended rate.

This might be a potentially breaking change, as the properties are no longer virtual. But anyone overwriting these was not getting what they wanted anyway - so perhaps a good thing they will get a compiler error and thus a chance to update their code to set the interval and other properties properly?


public override TimeSpan RedeliverInterval { get { return TimeSpan.FromMilliseconds(500); } }
RedeliverInterval = TimeSpan.FromMilliseconds(500);

This comment has been minimized.

Copy link
@ondrejpialek

ondrejpialek May 25, 2019

Author Contributor

This is a good example how these properties are now intended to be used; just set them anywhere from within the actor instead of overwriting.

var interval = new TimeSpan(RedeliverInterval.Ticks / 2);
_redeliverScheduleCancelable = _context.System.Scheduler.ScheduleTellRepeatedlyCancelable(interval, interval, _context.Self,
var delay = new TimeSpan(RedeliverInterval.Ticks / 2);
_redeliverScheduleCancelable = _context.System.Scheduler.ScheduleTellRepeatedlyCancelable(delay, RedeliverInterval, _context.Self,

This comment has been minimized.

Copy link
@ondrejpialek

ondrejpialek May 25, 2019

Author Contributor

Was this a bug? The first parameter is the initial delay of the very first callback. The second parameter is then the interval this will fire in, so this should be the whole RedeliverInterval, not half?

This comment has been minimized.

Copy link
@Aaronontheweb

Aaronontheweb Jun 21, 2019

Member

yeah, must have been a bug. Nice catch.

@Aaronontheweb

This comment has been minimized.

Copy link
Member

commented May 25, 2019

@Aaronontheweb

This comment has been minimized.

Copy link
Member

commented May 25, 2019

That'll get the failing unit test to pass.

@ondrejpialek

This comment has been minimized.

Copy link
Contributor Author

commented May 27, 2019

Sorry I haven't read that part of the docs. That explains that popup I saw and the files I had to exclude from the commit :) Will add it and update the PR.

ondrejpialek added some commits May 27, 2019

Merge API changes to approved for PR review
Delete the received file committed by mistake
@ondrejpialek

This comment has been minimized.

Copy link
Contributor Author

commented May 27, 2019

PR ready @Aaronontheweb

@Aaronontheweb

This comment has been minimized.

Copy link
Member

commented May 27, 2019

get => _redeliverInterval;
set
{
if (_redeliverInterval != value)

This comment has been minimized.

Copy link
@Horusiath

Horusiath May 29, 2019

Contributor

Doesn't this looks too surprising? I mean, using setter is usually considered to be simple operation with no side effect (except changing value type). The fact, that it resets schedule may be the source of problems, for example in debugging, in the future.

This comment has been minimized.

Copy link
@ondrejpialek

ondrejpialek May 29, 2019

Author Contributor

It's here so that the semantic behaves "correctly", e.g. if you set a different interval it should send things at a different interval. I agree that having too much logic in setters is not great though (thought I thought it's acceptable and tried to limit the resets by checking for a change).

What do you propose? Have a method instead for changing this? Or have this read only and only accept the value via ctor? Or making this smarter by calculating the correct delay between the elapsed time and new interval?

This comment has been minimized.

Copy link
@Horusiath

Horusiath May 30, 2019

Contributor

AFAIK those configuration values should be defined exactly once - when an actor is created. I'd probably need verify it somehow, but we're having a state machines here, so changing this value in different actor "state" may result in different side effects - and I'm not sure if all of them are acceptable.

Simplest way I can think of ATM to make it break the actor would be to call RedeliverInterval = x in actor's PostStop method after calling base.PostStop(), which is cancelling all scheduled messages. Any call like this one made afterwards would trigger it to schedule the message back again, but never actually cancel.

This comment has been minimized.

Copy link
@ondrejpialek

ondrejpialek May 30, 2019

Author Contributor

That would not happen the way it's currently written as it checks for _redeliverScheduleCancelable being active before restarting it.

if (_redeliverScheduleCancelable != null)
{
	_redeliverScheduleCancelable.Cancel();
	_redeliverScheduleCancelable = null;
	StartRedeliverTask();
}

But if you prefer not to have these mutable I can redo this PR in a different way - the values would be set via ctor only (at the cost of having to read the redelivery settings in two places (both AtLeastOnce actors that use this semantic).

The problem with that approach is that the AtLeastOnceDeliverySemantic is created in the actor's ctor and you should not access virtual fields inside a ctor, so the only way that comes in mind is specifying two ctors for each actor so that you can specify these parameters calling the right base ctor.

This comment has been minimized.

Copy link
@ondrejpialek

ondrejpialek May 30, 2019

Author Contributor

Actually there is already a ctor on the actors that takes the redelivery settings. So perhaps we make the properties non-virtual in order not to confuse the developers (as overwriting has no effect currently) and we fix the docs to say to use the overloaded ctor instead of overwriting?

This comment has been minimized.

Copy link
@Horusiath

Horusiath May 31, 2019

Contributor

It would be nice, but the problem is backwards compatibility. I think, we can stay with your current approach. Any thoughts @Aaronontheweb ?

This comment has been minimized.

Copy link
@Aaronontheweb

Aaronontheweb Jun 21, 2019

Member

@ondrejpialek @Horusiath

Actually there is already a ctor on the actors that takes the redelivery settings. So perhaps we make the properties non-virtual in order not to confuse the developers (as overwriting has no effect currently) and we fix the docs to say to use the overloaded ctor instead of overwriting?

Since this is going into the v1.4 branch and we can afford to take some more liberties with the API there - if overriding the properties already has no effect, I don't see an issue with removing the virtual modifier on those properties and just having developers use the constructor overload instead. I think making these settings immutable and settable through the constructor is probably the best way to go, in order to avoid having those side effects during the processing of the state machine as @Horusiath mentioned.

Can you go ahead with those changes @ondrejpialek ?


if (deliveriesAboveThreshold.Length > 0)
{
_context.Self.Tell(new UnconfirmedWarning(deliveriesAboveThreshold));

This comment has been minimized.

Copy link
@Horusiath

Horusiath May 29, 2019

Contributor

Same here, the last thing I would expect from using (this.WarnAfterNumberOfUnconfirmedAttempts = 5) is that it may send messages to actors and allocate extra arrays.

This comment has been minimized.

Copy link
@ondrejpialek

ondrejpialek May 29, 2019

Author Contributor

Again - this is to make the logic correct - if you start with 10, have 6 unconfirmed, then switch to 5 then you should receive the warning.

What do you think is best? Have this read only (and accept the value only via ctor?) or not trigger immediately but wait for next unconfirmed message?

This comment has been minimized.

Copy link
@Aaronontheweb

Aaronontheweb Jun 21, 2019

Member

Agree with @Horusiath - this should be immutable I think. Pick your strategy and stick with it at the time the actor starts.

@Aaronontheweb

This comment has been minimized.

Copy link
Member

commented Jun 3, 2019

@Horusiath saw that you tagged me here - my apologies for the delay! I'll take a look at this today.

@Aaronontheweb
Copy link
Member

left a comment

Need to make this immutable, but otherwise this is a very important and much-needed change.

@@ -16,11 +16,11 @@ namespace Akka.Persistence
{
protected AtLeastOnceDeliveryActor() { }
protected AtLeastOnceDeliveryActor(Akka.Persistence.PersistenceSettings.AtLeastOnceDeliverySettings settings) { }
public int MaxUnconfirmedMessages { get; }
public virtual System.TimeSpan RedeliverInterval { get; }

This comment has been minimized.

Copy link
@Aaronontheweb

Aaronontheweb Jun 21, 2019

Member

I'm ok with this change.

public int MaxUnconfirmedMessages
{
get => _atLeastOnceDeliverySemantic.MaxUnconfirmedMessages;
set => _atLeastOnceDeliverySemantic.MaxUnconfirmedMessages = value;

This comment has been minimized.

Copy link
@Aaronontheweb

Aaronontheweb Jun 21, 2019

Member

All of these settings should be immutable and overloadable via the CTOR, as you mentioned @ondrejpialek


if (deliveriesAboveThreshold.Length > 0)
{
_context.Self.Tell(new UnconfirmedWarning(deliveriesAboveThreshold));

This comment has been minimized.

Copy link
@Aaronontheweb

Aaronontheweb Jun 21, 2019

Member

Agree with @Horusiath - this should be immutable I think. Pick your strategy and stick with it at the time the actor starts.

var interval = new TimeSpan(RedeliverInterval.Ticks / 2);
_redeliverScheduleCancelable = _context.System.Scheduler.ScheduleTellRepeatedlyCancelable(interval, interval, _context.Self,
var delay = new TimeSpan(RedeliverInterval.Ticks / 2);
_redeliverScheduleCancelable = _context.System.Scheduler.ScheduleTellRepeatedlyCancelable(delay, RedeliverInterval, _context.Self,

This comment has been minimized.

Copy link
@Aaronontheweb

Aaronontheweb Jun 21, 2019

Member

yeah, must have been a bug. Nice catch.

ondrejpialek added some commits Jul 12, 2019

Making various AtLeastOnceDelivery settings read-only
Added a .ctor overload to AtLeastOnceDelivery actors that allows to tweak default settings so that the user does not have to construct and specify all settings manually.
@ondrejpialek
Copy link
Contributor Author

left a comment

Hello @Aaronontheweb @Horusiath,

I have updated my PR and removed the setters from the properties. I added a convenience constructor overload for the actors to simplify tweaking the settings from a parent constructor call. Let me know what you think (see comments in the relevant code).

Cheers,
Ondrej.

@@ -136,7 +136,8 @@ internal class ChaosSender : AtLeastOnceDeliveryActor

public ILoggingAdapter Log { get { return _log ?? (_log = Context.GetLogger()); }}

public ChaosSender(IActorRef destination, IActorRef probe)
public ChaosSender(IActorRef destination, IActorRef probe)
: base(x => x.WithRedeliverInterval(TimeSpan.FromMilliseconds(500)))

This comment has been minimized.

Copy link
@ondrejpialek

ondrejpialek Jul 12, 2019

Author Contributor

This is a new ctor overload, see comment on the base class

@Aaronontheweb Aaronontheweb self-requested a review Jul 12, 2019

@Aaronontheweb
Copy link
Member

left a comment

LGTM - going to wait on CI

@Aaronontheweb Aaronontheweb added this to the 1.4.0 milestone Jul 12, 2019

@Aaronontheweb

This comment has been minimized.

Copy link
Member

commented Jul 12, 2019

@Horusiath I'm good with this - any objections?

Aaronontheweb added some commits Jul 16, 2019

@Horusiath Horusiath self-requested a review Jul 20, 2019

@Horusiath Horusiath merged commit 2c78120 into akkadotnet:dev Jul 20, 2019

8 checks passed

PR Validation Build #2019.7.19.2 succeeded
Details
PR Validation (.NET Core Multi-Node Tests (Windows)) .NET Core Multi-Node Tests (Windows) succeeded
Details
PR Validation (.NET Core Unit Tests (Linux)) .NET Core Unit Tests (Linux) succeeded
Details
PR Validation (.NET Core Unit Tests (Windows)) .NET Core Unit Tests (Windows) succeeded
Details
PR Validation (.NET Framework Multi-Node Tests (Windows)) .NET Framework Multi-Node Tests (Windows) succeeded
Details
PR Validation (.NET Framework Unit Tests (Windows)) .NET Framework Unit Tests (Windows) succeeded
Details
PR Validation (NuGet Pack) NuGet Pack succeeded
Details
PR Validation (Windows Build) Windows Build succeeded
Details

Aaronontheweb added a commit to Aaronontheweb/akka.net that referenced this pull request Jul 26, 2019

Allow AtLeastOnceDelivery parameters to be set from deriving classes …
…(as intended) (akkadotnet#3810)

* Allow various redelivery parameters to be tweaked from derived classes

* Adding API change declaration

* Merge API changes to approved for PR review

Delete the received file committed by mistake

* Making various AtLeastOnceDelivery settings read-only

Added a .ctor overload to AtLeastOnceDelivery actors that allows to tweak default settings so that the user does not have to construct and specify all settings manually.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.