You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ofbiz.apache.org by Adrian Crum <ad...@sandglass-software.com> on 2014/08/31 10:50:34 UTC
Discussion: Service Engine Refactoring
Currently, the service engine classes are a bit muddled.
GenericDispatcher (the default LocalDispatcher implementation) and
DispatchContext have feature envy. Some functionality in DispatchContext
belongs in ServiceDispatcher.
I would like to clean this up a bit and provide better separation of
concerns. The service engine API will not change - the work will be done
"under the hood."
My hope is to get the code to a point where DispatchContext becomes less
integral to its neighbors, and it becomes more of a single-use
lightweight container. Again, this will not change the API at all.
However, it will open up the possibility to improve the API. For
example, instead of this service implementation:
public static Map<String, Object> myService(DispatchContext dctx,
Map<String, Object> context) {
Locale locale = (Locale) context.get("locale");
...
we could have:
public static Map<String, Object> myService(DispatchContext dctx) {
Locale locale = dctx.getParameter("locale");
...
What do you think?
--
Adrian Crum
Sandglass Software
www.sandglass-software.com
Re: Discussion: Service Engine Refactoring
Posted by Adrian Crum <ad...@sandglass-software.com>.
Here is some ASCII art that might help:
DispatchContext (a container for objects needed by service implementations)
|
|__ (references) LocalDispatcher (an application-specific service
dispatcher)
|
|__ (delegates to) ServiceDispatcher
|
|__ (based on) Delegator
Adrian Crum
Sandglass Software
www.sandglass-software.com
On 8/31/2014 10:36 AM, Jacopo Cappellato wrote:
> Thank you Adrian,
>
> please see inline:
>
> On Aug 31, 2014, at 10:50 AM, Adrian Crum <ad...@sandglass-software.com> wrote:
>
>> Currently, the service engine classes are a bit muddled. GenericDispatcher (the default LocalDispatcher implementation) and DispatchContext have feature envy. Some functionality in DispatchContext belongs in ServiceDispatcher.
>>
>> I would like to clean this up a bit and provide better separation of concerns. The service engine API will not change - the work will be done "under the hood."
>>
>> My hope is to get the code to a point where DispatchContext becomes less integral to its neighbors, and it becomes more of a single-use lightweight container. Again, this will not change the API at all.
>
> I think it would help the other committers to provide feedback if you could (in just a few sentences) summarize the following:
> a) what is the original concern of DispatchContext and of GenericDispatcher
> b) where do you see that this concerns are not well implemented in the code
> c) if you would like to modify #a, quickly describe the new purposes of the classes in your vision
> d) what are the methods/fields that you would like to move from DispatchContext
>
> Thanks
>
> Jacopo
>
> PS: in general I also see a lot of code that should be cleaned in DispatchContext; for example, the initialization of the static map in the constructor, the method getAllServiceNames() that should be static, bad thread-safe implementation, probably the modelServiceMapByModel should not even be there...
>
>>
>> However, it will open up the possibility to improve the API. For example, instead of this service implementation:
>>
>> public static Map<String, Object> myService(DispatchContext dctx, Map<String, Object> context) {
>> Locale locale = (Locale) context.get("locale");
>> ...
>>
>> we could have:
>>
>> public static Map<String, Object> myService(DispatchContext dctx) {
>> Locale locale = dctx.getParameter("locale");
>> ...
>>
>>
>> What do you think?
>>
>>
>> --
>> Adrian Crum
>> Sandglass Software
>> www.sandglass-software.com
>
Re: Discussion: Service Engine Refactoring
Posted by Adrian Crum <ad...@sandglass-software.com>.
Inline...
Adrian Crum
Sandglass Software
www.sandglass-software.com
On 8/31/2014 10:36 AM, Jacopo Cappellato wrote:
> Thank you Adrian,
>
> please see inline:
>
> On Aug 31, 2014, at 10:50 AM, Adrian Crum <ad...@sandglass-software.com> wrote:
>
>> Currently, the service engine classes are a bit muddled. GenericDispatcher (the default LocalDispatcher implementation) and DispatchContext have feature envy. Some functionality in DispatchContext belongs in ServiceDispatcher.
>>
>> I would like to clean this up a bit and provide better separation of concerns. The service engine API will not change - the work will be done "under the hood."
>>
>> My hope is to get the code to a point where DispatchContext becomes less integral to its neighbors, and it becomes more of a single-use lightweight container. Again, this will not change the API at all.
>
> I think it would help the other committers to provide feedback if you could (in just a few sentences) summarize the following:
> a) what is the original concern of DispatchContext and of GenericDispatcher
I replied to this in another post.
> b) where do you see that this concerns are not well implemented in the code
> c) if you would like to modify #a, quickly describe the new purposes of the classes in your vision
DispatchContext becomes a lightweight container for objects that a
service implementation needs. Instead of relatively-constant instances
being kept inside a GenericDispatcher, a new instance is created
on-the-fly whenever a service is invoked.
> d) what are the methods/fields that you would like to move from DispatchContext
It's the other way around. I want to remove the DispatchContext
reference from GenericDispatcher.
>
> Thanks
>
> Jacopo
>
> PS: in general I also see a lot of code that should be cleaned in DispatchContext; for example, the initialization of the static map in the constructor, the method getAllServiceNames() that should be static, bad thread-safe implementation, probably the modelServiceMapByModel should not even be there...
Exactly. From my perspective, that code should be in ServiceDispatcher.
>
>>
>> However, it will open up the possibility to improve the API. For example, instead of this service implementation:
>>
>> public static Map<String, Object> myService(DispatchContext dctx, Map<String, Object> context) {
>> Locale locale = (Locale) context.get("locale");
>> ...
>>
>> we could have:
>>
>> public static Map<String, Object> myService(DispatchContext dctx) {
>> Locale locale = dctx.getParameter("locale");
>> ...
>>
>>
>> What do you think?
>>
>>
>> --
>> Adrian Crum
>> Sandglass Software
>> www.sandglass-software.com
>
Re: Discussion: Service Engine Refactoring
Posted by Jacques Le Roux <ja...@les7arts.com>.
For now there is only a link to services.html from the tutorial.
The best place to add another link seems https://cwiki.apache.org/confluence/display/OFBTECH/Service+Engine+Guide
Jacques
Le 31/08/2014 12:14, Pierre @GMail a écrit :
> That is a nice piece of explanation. Should be part of the documentation, if it is not already.
>
> Regards,
>
> Pierre
>
> Sent from my iPhone
>
>> On 31 aug. 2014, at 11:56, Jacopo Cappellato <ja...@hotwaxmedia.com> wrote:
>>
>>
>>> On Aug 31, 2014, at 11:36 AM, Jacopo Cappellato <ja...@hotwaxmedia.com> wrote:
>>>
>>> a) what is the original concern of DispatchContext and of GenericDispatcher
>> This comes from this old but still interesting document: http://ofbiz.apache.org/docs/services.html
>>
>> ========================================
>> *Service Dispatcher*
>>
>> The Service Dispatcher handles dispatching services to the appropriate Service Engine where it is then invoked. There is exactly one ServiceDispatcher for each Entity Delegator. If there are multiple delegators in an application there will also be multiple dispatchers. The ServiceDispatcher is accessed via a LocalDispatcher. There can be many LocalDispatchers associated with a ServiceDispatcher. Each LocalDispatcher is uniquely named and contains its own list of service definitions. When creating an instance of a LocalDispatcher, a DispatchContext is also created and passed to the ServiceEngine.
>>
>> A LocalDispatcher is associated with an application. Applications never talk directly to the ServiceDispatcher. The LocalDispatcher contains an API for invoking services, which are routed through the ServiceDispather. However, applications may be running in different threads than the actual ServiceDispatcher, so it is left to the LocalDispatcher to keep a DispatchContext which among other things keeps a reference to the applications classloader.
>>
>> *Dispatch Context*
>>
>> The DispatchContext is created by the LocalDispatcher upon instantiation. This is the runtime dispatcher context. It contains necessary information to process services for each dispatcher. This context contains the reference to each of the service definition files, the classloader which should be used for invocation, a reference to the delegator and its dispatcher along with a 'bag' of user defined attributes. This context is passed on to each service when invoked and is used by the dispatcher to determine the service's model.
>>
>> ========================================
>>
>> Jacopo
>
Re: Discussion: Service Engine Refactoring
Posted by "Pierre @GMail" <pi...@gmail.com>.
That is a nice piece of explanation. Should be part of the documentation, if it is not already.
Regards,
Pierre
Sent from my iPhone
> On 31 aug. 2014, at 11:56, Jacopo Cappellato <ja...@hotwaxmedia.com> wrote:
>
>
>> On Aug 31, 2014, at 11:36 AM, Jacopo Cappellato <ja...@hotwaxmedia.com> wrote:
>>
>> a) what is the original concern of DispatchContext and of GenericDispatcher
>
> This comes from this old but still interesting document: http://ofbiz.apache.org/docs/services.html
>
> ========================================
> *Service Dispatcher*
>
> The Service Dispatcher handles dispatching services to the appropriate Service Engine where it is then invoked. There is exactly one ServiceDispatcher for each Entity Delegator. If there are multiple delegators in an application there will also be multiple dispatchers. The ServiceDispatcher is accessed via a LocalDispatcher. There can be many LocalDispatchers associated with a ServiceDispatcher. Each LocalDispatcher is uniquely named and contains its own list of service definitions. When creating an instance of a LocalDispatcher, a DispatchContext is also created and passed to the ServiceEngine.
>
> A LocalDispatcher is associated with an application. Applications never talk directly to the ServiceDispatcher. The LocalDispatcher contains an API for invoking services, which are routed through the ServiceDispather. However, applications may be running in different threads than the actual ServiceDispatcher, so it is left to the LocalDispatcher to keep a DispatchContext which among other things keeps a reference to the applications classloader.
>
> *Dispatch Context*
>
> The DispatchContext is created by the LocalDispatcher upon instantiation. This is the runtime dispatcher context. It contains necessary information to process services for each dispatcher. This context contains the reference to each of the service definition files, the classloader which should be used for invocation, a reference to the delegator and its dispatcher along with a 'bag' of user defined attributes. This context is passed on to each service when invoked and is used by the dispatcher to determine the service's model.
>
> ========================================
>
> Jacopo
Re: Discussion: Service Engine Refactoring
Posted by Adrian Crum <ad...@sandglass-software.com>.
That last paragraph describes the cleanup I want to do. If a
LocalDispatcher contains things specific to an application, then why are
some of those things kept in GenericDispatcher and others are kept in
DispatchContext? This is the feature-envy part - they are both trying to
be the same thing.
It would simpler and cleaner if we keep application-specific bits in
GenericDispatcher, and just have DispatchContext reference it.
Adrian Crum
Sandglass Software
www.sandglass-software.com
On 8/31/2014 10:56 AM, Jacopo Cappellato wrote:
>
> On Aug 31, 2014, at 11:36 AM, Jacopo Cappellato <ja...@hotwaxmedia.com> wrote:
>
>> a) what is the original concern of DispatchContext and of GenericDispatcher
>
> This comes from this old but still interesting document: http://ofbiz.apache.org/docs/services.html
>
> ========================================
> *Service Dispatcher*
>
> The Service Dispatcher handles dispatching services to the appropriate Service Engine where it is then invoked. There is exactly one ServiceDispatcher for each Entity Delegator. If there are multiple delegators in an application there will also be multiple dispatchers. The ServiceDispatcher is accessed via a LocalDispatcher. There can be many LocalDispatchers associated with a ServiceDispatcher. Each LocalDispatcher is uniquely named and contains its own list of service definitions. When creating an instance of a LocalDispatcher, a DispatchContext is also created and passed to the ServiceEngine.
>
> A LocalDispatcher is associated with an application. Applications never talk directly to the ServiceDispatcher. The LocalDispatcher contains an API for invoking services, which are routed through the ServiceDispather. However, applications may be running in different threads than the actual ServiceDispatcher, so it is left to the LocalDispatcher to keep a DispatchContext which among other things keeps a reference to the applications classloader.
>
> *Dispatch Context*
>
> The DispatchContext is created by the LocalDispatcher upon instantiation. This is the runtime dispatcher context. It contains necessary information to process services for each dispatcher. This context contains the reference to each of the service definition files, the classloader which should be used for invocation, a reference to the delegator and its dispatcher along with a 'bag' of user defined attributes. This context is passed on to each service when invoked and is used by the dispatcher to determine the service's model.
>
> ========================================
>
> Jacopo
>
Re: Discussion: Service Engine Refactoring
Posted by Jacopo Cappellato <ja...@hotwaxmedia.com>.
On Aug 31, 2014, at 11:36 AM, Jacopo Cappellato <ja...@hotwaxmedia.com> wrote:
> a) what is the original concern of DispatchContext and of GenericDispatcher
This comes from this old but still interesting document: http://ofbiz.apache.org/docs/services.html
========================================
*Service Dispatcher*
The Service Dispatcher handles dispatching services to the appropriate Service Engine where it is then invoked. There is exactly one ServiceDispatcher for each Entity Delegator. If there are multiple delegators in an application there will also be multiple dispatchers. The ServiceDispatcher is accessed via a LocalDispatcher. There can be many LocalDispatchers associated with a ServiceDispatcher. Each LocalDispatcher is uniquely named and contains its own list of service definitions. When creating an instance of a LocalDispatcher, a DispatchContext is also created and passed to the ServiceEngine.
A LocalDispatcher is associated with an application. Applications never talk directly to the ServiceDispatcher. The LocalDispatcher contains an API for invoking services, which are routed through the ServiceDispather. However, applications may be running in different threads than the actual ServiceDispatcher, so it is left to the LocalDispatcher to keep a DispatchContext which among other things keeps a reference to the applications classloader.
*Dispatch Context*
The DispatchContext is created by the LocalDispatcher upon instantiation. This is the runtime dispatcher context. It contains necessary information to process services for each dispatcher. This context contains the reference to each of the service definition files, the classloader which should be used for invocation, a reference to the delegator and its dispatcher along with a 'bag' of user defined attributes. This context is passed on to each service when invoked and is used by the dispatcher to determine the service's model.
========================================
Jacopo
Re: Discussion: Service Engine Refactoring
Posted by Jacopo Cappellato <ja...@hotwaxmedia.com>.
Thank you Adrian,
please see inline:
On Aug 31, 2014, at 10:50 AM, Adrian Crum <ad...@sandglass-software.com> wrote:
> Currently, the service engine classes are a bit muddled. GenericDispatcher (the default LocalDispatcher implementation) and DispatchContext have feature envy. Some functionality in DispatchContext belongs in ServiceDispatcher.
>
> I would like to clean this up a bit and provide better separation of concerns. The service engine API will not change - the work will be done "under the hood."
>
> My hope is to get the code to a point where DispatchContext becomes less integral to its neighbors, and it becomes more of a single-use lightweight container. Again, this will not change the API at all.
I think it would help the other committers to provide feedback if you could (in just a few sentences) summarize the following:
a) what is the original concern of DispatchContext and of GenericDispatcher
b) where do you see that this concerns are not well implemented in the code
c) if you would like to modify #a, quickly describe the new purposes of the classes in your vision
d) what are the methods/fields that you would like to move from DispatchContext
Thanks
Jacopo
PS: in general I also see a lot of code that should be cleaned in DispatchContext; for example, the initialization of the static map in the constructor, the method getAllServiceNames() that should be static, bad thread-safe implementation, probably the modelServiceMapByModel should not even be there...
>
> However, it will open up the possibility to improve the API. For example, instead of this service implementation:
>
> public static Map<String, Object> myService(DispatchContext dctx, Map<String, Object> context) {
> Locale locale = (Locale) context.get("locale");
> ...
>
> we could have:
>
> public static Map<String, Object> myService(DispatchContext dctx) {
> Locale locale = dctx.getParameter("locale");
> ...
>
>
> What do you think?
>
>
> --
> Adrian Crum
> Sandglass Software
> www.sandglass-software.com
Re: Discussion: Service Engine Refactoring
Posted by Adrian Crum <ad...@sandglass-software.com>.
That's fine. The opportunity will be there if anyone wants to explore
it. It is not something I will push for.
Adrian Crum
Sandglass Software
www.sandglass-software.com
On 8/31/2014 10:54 AM, Jacopo Cappellato wrote:
>
> On Aug 31, 2014, at 10:50 AM, Adrian Crum <ad...@sandglass-software.com> wrote:
>
>> However, it will open up the possibility to improve the API. For example, instead of this service implementation:
>>
>> public static Map<String, Object> myService(DispatchContext dctx, Map<String, Object> context) {
>> Locale locale = (Locale) context.get("locale");
>> ...
>>
>> we could have:
>>
>> public static Map<String, Object> myService(DispatchContext dctx) {
>> Locale locale = dctx.getParameter("locale");
>> ...
>
> DispatchContext and the Map with input parameters have a completely different purpose and I don't think this would be an "improvement" per se. We will have to revisit this conversation at some point to be sure we are in the same page.
>
> Jacopo
>
Re: Discussion: Service Engine Refactoring
Posted by Jacopo Cappellato <ja...@hotwaxmedia.com>.
On Aug 31, 2014, at 10:50 AM, Adrian Crum <ad...@sandglass-software.com> wrote:
> However, it will open up the possibility to improve the API. For example, instead of this service implementation:
>
> public static Map<String, Object> myService(DispatchContext dctx, Map<String, Object> context) {
> Locale locale = (Locale) context.get("locale");
> ...
>
> we could have:
>
> public static Map<String, Object> myService(DispatchContext dctx) {
> Locale locale = dctx.getParameter("locale");
> ...
DispatchContext and the Map with input parameters have a completely different purpose and I don't think this would be an "improvement" per se. We will have to revisit this conversation at some point to be sure we are in the same page.
Jacopo
Re: Discussion: Service Engine Refactoring
Posted by Adrian Crum <ad...@sandglass-software.com>.
https://issues.apache.org/jira/browse/OFBIZ-5743
Adrian Crum
Sandglass Software
www.sandglass-software.com
On 9/1/2014 7:26 AM, Jacopo Cappellato wrote:
> I would like to review your work; could you please commit the changes to an experimental branch or at least share a patch in Jira?
>
> Thanks,
>
> Jacopo
>
> On Aug 31, 2014, at 8:35 PM, Adrian Crum <ad...@sandglass-software.com> wrote:
>
>> I have this done. The change is quite subtle, and it will pave the way for removing GenericDelegator references from the service engine.
>>
>> I will wait a few days before committing so others have time to respond.
>>
>> Adrian Crum
>> Sandglass Software
>> www.sandglass-software.com
>>
>> On 8/31/2014 9:50 AM, Adrian Crum wrote:
>>> Currently, the service engine classes are a bit muddled.
>>> GenericDispatcher (the default LocalDispatcher implementation) and
>>> DispatchContext have feature envy. Some functionality in DispatchContext
>>> belongs in ServiceDispatcher.
>>>
>>> I would like to clean this up a bit and provide better separation of
>>> concerns. The service engine API will not change - the work will be done
>>> "under the hood."
>>>
>>> My hope is to get the code to a point where DispatchContext becomes less
>>> integral to its neighbors, and it becomes more of a single-use
>>> lightweight container. Again, this will not change the API at all.
>>>
>>> However, it will open up the possibility to improve the API. For
>>> example, instead of this service implementation:
>>>
>>> public static Map<String, Object> myService(DispatchContext dctx,
>>> Map<String, Object> context) {
>>> Locale locale = (Locale) context.get("locale");
>>> ...
>>>
>>> we could have:
>>>
>>> public static Map<String, Object> myService(DispatchContext dctx) {
>>> Locale locale = dctx.getParameter("locale");
>>> ...
>>>
>>>
>>> What do you think?
>>>
>>>
>
Re: Discussion: Service Engine Refactoring
Posted by Jacopo Cappellato <ja...@hotwaxmedia.com>.
I would like to review your work; could you please commit the changes to an experimental branch or at least share a patch in Jira?
Thanks,
Jacopo
On Aug 31, 2014, at 8:35 PM, Adrian Crum <ad...@sandglass-software.com> wrote:
> I have this done. The change is quite subtle, and it will pave the way for removing GenericDelegator references from the service engine.
>
> I will wait a few days before committing so others have time to respond.
>
> Adrian Crum
> Sandglass Software
> www.sandglass-software.com
>
> On 8/31/2014 9:50 AM, Adrian Crum wrote:
>> Currently, the service engine classes are a bit muddled.
>> GenericDispatcher (the default LocalDispatcher implementation) and
>> DispatchContext have feature envy. Some functionality in DispatchContext
>> belongs in ServiceDispatcher.
>>
>> I would like to clean this up a bit and provide better separation of
>> concerns. The service engine API will not change - the work will be done
>> "under the hood."
>>
>> My hope is to get the code to a point where DispatchContext becomes less
>> integral to its neighbors, and it becomes more of a single-use
>> lightweight container. Again, this will not change the API at all.
>>
>> However, it will open up the possibility to improve the API. For
>> example, instead of this service implementation:
>>
>> public static Map<String, Object> myService(DispatchContext dctx,
>> Map<String, Object> context) {
>> Locale locale = (Locale) context.get("locale");
>> ...
>>
>> we could have:
>>
>> public static Map<String, Object> myService(DispatchContext dctx) {
>> Locale locale = dctx.getParameter("locale");
>> ...
>>
>>
>> What do you think?
>>
>>
Re: Discussion: Service Engine Refactoring
Posted by Adrian Crum <ad...@sandglass-software.com>.
I have this done. The change is quite subtle, and it will pave the way
for removing GenericDelegator references from the service engine.
I will wait a few days before committing so others have time to respond.
Adrian Crum
Sandglass Software
www.sandglass-software.com
On 8/31/2014 9:50 AM, Adrian Crum wrote:
> Currently, the service engine classes are a bit muddled.
> GenericDispatcher (the default LocalDispatcher implementation) and
> DispatchContext have feature envy. Some functionality in DispatchContext
> belongs in ServiceDispatcher.
>
> I would like to clean this up a bit and provide better separation of
> concerns. The service engine API will not change - the work will be done
> "under the hood."
>
> My hope is to get the code to a point where DispatchContext becomes less
> integral to its neighbors, and it becomes more of a single-use
> lightweight container. Again, this will not change the API at all.
>
> However, it will open up the possibility to improve the API. For
> example, instead of this service implementation:
>
> public static Map<String, Object> myService(DispatchContext dctx,
> Map<String, Object> context) {
> Locale locale = (Locale) context.get("locale");
> ...
>
> we could have:
>
> public static Map<String, Object> myService(DispatchContext dctx) {
> Locale locale = dctx.getParameter("locale");
> ...
>
>
> What do you think?
>
>