-
Notifications
You must be signed in to change notification settings - Fork 30
IOTA-41: Spring dependency injection for fey performers #35
base: master
Are you sure you want to change the base?
Conversation
@cristofolini can you add IOTA-41 in the PR description? Because of the missing "-" Jira is not linking this PR to the ticket. Also, what about creating an example of Fey actors using spring? |
@rafaelweingartner I've changed the PR title to include the "-" and now the JIRA ticket is linking to it. I think an example with Spring actors is a pretty good idea. Let me think something up. |
@cristofolini Could you write some unit tests related to this PR? |
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 have a doubt on the type of autowiring it is being used, can you clarify @cristofolini?
|
||
override def onStart(): Unit = { | ||
super.onStart() | ||
factory.autowireBeanProperties(this, AutowireCapableBeanFactory.AUTOWIRE_BY_NAME, true) |
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.
By name here will force us to use the @qualifier, right?
What about using by type?
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.
You'd only need a @qualifier if the bean id in the XML and the autowired attribute's name are different.
The difference between by_name and by_type is exactly what it says on the tin. By_name will autowire beans with an id that matches the name, while by_type will look for a bean of the same type. I don't really have strong reasons to go either way. Do you think it might be better to go by_type?
If so I can change that when I get the tests in.
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 do find the by_type better, because then we do not need to care about variable names and object names/ids.
@barbaragomes I've added a couple unit tests, one to check that a context is loaded and one to check that a factory is instatiated. I want to get some feedback on two things:
@rafaelweingartner I've changed the autowiring method to by_type. |
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.
@cristofolini and @barbaragomes,
I really do not understand the test here. It may be due to the fact that Scale looks like an alien language for me, so pardon me if my doubt is silly.
The unit test I see we can write here is regarding the method “getPerformer” from “Ensemble.scala” that Lucas changed. So, if I were to code the test in Java, I would create two tests. One where the parameter “performerInfo.contextPath” is null (maybe others for when the parameter is blank or empty), and check if the code used to instantiate the agent is the code at line 237 (Props method? Object creation?!). Later, I would to a test where the parameter “performerInfo.contextPath” is not empty and check if the code at line 236 is executed. By check if the code is executed, I mean execute a mockito.verify method or a powerMockito.verify calls; or Maybe checking if the result of the method is the result we expected.
However, I do not see that in the current test code. Can you explain the idea behind the test and where the assertions are?
@rafaelweingartner All it does right now is assert that an ApplicationContext and a BeanFactory are created when a proper contextPath is present. I agree that we should have tests for when it isn't. I can see about writing those as well. |
Just to update everyone here. This PR passed in out simple test cases scenarios, but when we started using it in a production system, it did not work properly. There is a problem on how the class loaders of different agents are being managed. We are working to fix it and then add it here. |
Our proposed implementation consists of creating a class that extends the original FeyGenericActor (named FeyGenericSpringActor), which allows for one more argument to be passed within its constructor, which is a String containing a path for the XML file containing that actor's Spring context, which can be an absolute or relative filesystem path (relative to the current VM working directory) or obtained from the classpath (by specifying a "classpath:" prefix); other protocols and prefixes may be supported by the Spring framework. The new actor class has two attributes: an ApplicationContext and its AutowireCapableBeanFactory, which will allow us to instantiate the beans specified in the actor's context.
Other modifications to the fey engine core include the necessary changes related to parsing the orchestration JSON: adding an entry for the actor context's file path on the JSON validator and parser method, along with a conditional check to see if a path to the actor's context was specified in the JSON, so that fey will instantiate a FeyGenericSpringActor instead of a FeyGenericActor.
Lastly, we've created a FeyApplicationContext.xml, which should be the parent context to all actor contexts created, and will contain useful universal declarations. For now, all it does is enable annotation usage in all Spring actors.