Re: Discussion: Service Engine Refactoring
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
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 myService(DispatchContext dctx,
Map context) {
Locale locale = (Locale) context.get("locale");
...
we could have:
public static Map myService(DispatchContext dctx) {
Locale locale = dctx.getParameter("locale");
...
What do you think?
Re: Discussion: Service Engine Refactoring
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
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 myService(DispatchContext dctx,
>> Map context) {
>> Locale locale = (Locale) context.get("locale");
>> ...
>>
>> we could have:
>>
>> public static Map myService(DispatchContext dctx) {
>> Locale locale = dctx.getParameter("locale");
>> ...
>>
>>
>> What do you think?
>>
>>
Re: Discussion: Service Engine Refactoring
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 myService(DispatchContext dctx,
Map context) {
Locale locale = (Locale) context.get("locale");
...
we could have:
public static Map myService(DispatchContext dctx) {
Locale locale = dctx.getParameter("locale");
...
What do you think?
Re: Discussion: Service Engine Refactoring
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
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 myService(DispatchContext dctx, Map context) {
Locale locale = (Locale) context.get("locale");
...
we could have:
public static Map 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
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 wrote: On Aug 31, 2014, at 11:36 AM, Jacopo Cappellato 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
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
wrote:
However, it will open up the possibility to improve the API. For example,
instead of this service implementation:
public static Map myService(DispatchContext dctx, Map context) {
Locale locale = (Locale) context.get("locale");
...
we could have:
public static Map 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
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 > wrote: > > >> On Aug 31, 2014, at 11:36 AM, Jacopo Cappellato >> 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
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
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 myService(DispatchContext dctx, Map context) {
Locale locale = (Locale) context.get("locale");
...
we could have:
public static Map 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
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 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
On Aug 31, 2014, at 11:36 AM, Jacopo Cappellato 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
On Aug 31, 2014, at 10:50 AM, Adrian Crum
wrote:
> However, it will open up the possibility to improve the API. For example,
> instead of this service implementation:
>
> public static Map myService(DispatchContext dctx, Map Object> context) {
> Locale locale = (Locale) context.get("locale");
> ...
>
> we could have:
>
> public static Map 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
Thank you Adrian,
please see inline:
On Aug 31, 2014, at 10: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.
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 myService(DispatchContext dctx, Map Object> context) {
> Locale locale = (Locale) context.get("locale");
> ...
>
> we could have:
>
> public static Map myService(DispatchContext dctx) {
> Locale locale = dctx.getParameter("locale");
> ...
>
>
> What do you think?
>
>
> --
> Adrian Crum
> Sandglass Software
> www.sandglass-software.com
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 myService(DispatchContext dctx,
Map context) {
Locale locale = (Locale) context.get("locale");
...
we could have:
public static Map myService(DispatchContext dctx) {
Locale locale = dctx.getParameter("locale");
...
What do you think?
--
Adrian Crum
Sandglass Software
www.sandglass-software.com
