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

Support akka.actor.guardian-supervisor-strategy. #1525

Closed
wants to merge 2 commits into from
Closed

Support akka.actor.guardian-supervisor-strategy. #1525

wants to merge 2 commits into from

Conversation

akoshelev
Copy link

Fixed issue #1524. LocalActorRefProvider now supports option 'akka.actor.guardian-supervisor-strategy'

@akoshelev
Copy link
Author

I'm a little bit worried about Pigeon.conf modification as it breaks backward compatibility. May be we should support lowercase syntax for types as well?

@rogeralsing
Copy link
Contributor

I'm a little bit worried about Pigeon.conf modification as it breaks backward compatibility. May be we should support lowercase syntax for types as well?

I dont think this is a real problem as the old value was never used.
Unlikely that anyone have a dependency on a non used value IMO.

if (supervisorStrategyType == null)
{
// TODO: is it ok to throw exception here?
throw new ArgumentException(string.Format("Type \"{0}\" is not found.", settings.SupervisorStrategyClass));
Copy link
Contributor

Choose a reason for hiding this comment

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

We throw Akka.ConfigurationException if the source of the value is the HOCON config.
ArgumentException is more of a user provided value.

There is also a third option here and use Type.GetType(settings.SupervisorStrategyClass,true)
Which will throw one of the following exceptions : https://msdn.microsoft.com/en-us/library/a87che9d(v=vs.110).aspx

I am leaning towards the latter as it would be the most idiomatic .NET way to deal with missing types

@akoshelev
Copy link
Author

Thanks @rogeralsing for the remark. I've changed the code, you may review the updated version now.
It seems though, we may support lowercase types in HOCON configuration out of the box, by setting the third parameter of GetType method to true

Type.GetType(settings.SupervisorStrategyClass, true, true)

But it's up to you.

@marcpiechura
Copy link
Contributor

@akoshelev there are also two missing tests in the ActorSystemSpec #60 about the supervisor strategy, maybe it would be make sense to include them in this PR?

You can find them in the scala version here and here.

@akoshelev
Copy link
Author

@Silv3rcircl3 Yep, I like the idea. Will do

@akoshelev
Copy link
Author

@Silv3rcircl3 @rogeralsing @Aaronontheweb I've got one problem with these tests. As I'm creating an actor system inside the test method, I'm not able to use EventFilter to filter Error messages - this filter works with Sys ActorSystem only. So formally tests are incomplete. Could you guys help me find a way to make this work?

@marcpiechura marcpiechura mentioned this pull request Dec 11, 2015
@Aaronontheweb
Copy link
Member

@akoshelev so we can't apply the EventFilter to a second ActorSystem created in the spec? Am I understanding you correctly?

@akoshelev
Copy link
Author

@Aaronontheweb Yes, you're right. EventFilter property of TestKit instance works with Sys actor only. This behaviour is defined in InternalEventFilterApplier that is used by EventFilterFactory. This class uses TestKit.Sys property to filter events.

        public T ExpectOne<T>(Func<T> func)
        {
            return Intercept(func, _testkit.Sys, null, 1);
        }

return Intercept(func, _testkit.Sys, null, 1);

@Aaronontheweb
Copy link
Member

@akoshelev in that case, I guess we'll need to add an overload to the EventFilter to take an external ActorSystem. I had to do the same thing with either the TestProbe or something else a while ago. May need to do that on a separate PR.

@akoshelev
Copy link
Author

@Aaronontheweb Ok, I'll try to add this overload and will update this PR accordingly.

Alex Koshelev added 2 commits January 6, 2016 22:09
Fixed issue #1524. LocalActorRefProvider now supports option 'akka.actor.guardian-supervisor-strategy'
Adds some missing tests from original ActorSystemSpec, details may be
found #60
@Aaronontheweb
Copy link
Member

Implemented by #1746, but we still appreciate #1753!

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.

None yet

4 participants