LocalDipatcher vs ServiceDispatcher

[Jacopo Cappellato-4]

Before we continue with the refactoring of LocalDipatcher vs ServiceDispatcher there is an issue that needs to be addressed: I suspect it was introduced when the LocalDispatcher/GenericDispatcher were introduced (after the ServiceDispatcher).

Premise: the details below are based on the recent refactoring I (and Adrian) did; however the same layout/structure/issues were also the same before the refactoring; the refactoring simply made it clearer and easier to read this package.

Here are some details:

1) the system creates one instance of the ServiceDispatcher class for each delegator
2) each instance of ServiceDispatcher holds a reference to important objects like the JobManager (runs the JobSandbox), the Security, the JmsListenerFactory and others; also these objects are created/cached as one per delegator
3) within each instance the system registers several DispatchContext/LocalDispatcher objects; this happens when we get a LocalDispatcher:

LocalDispatcher dispatcher  = ServiceContainer.getLocalDispatcher(String dispatcherName, Delegator delegator); // get from cache or create and add to cache an instance of ServiceDispatcher associated to the Delegator; then get or create and register into the ServiceDispatcher the DispatchContext/LocalDispatcher object with name dispatcherName

4) as a consequence of #1 and #3:
4.a) in order to uniquely identify a LocalDispatcher/DispatchContext in the system you need the pair (delegator, dispatcherName)
4.b) there are a few instances of ServiceDispatcher (one per delegator) and several instances of LocalDispatcher/DispatchContext

5) the only implementation we have of LocalDispatcher is the GenericDispatcher/GenericAbstractDispatcher; the status maintained by these objects (i.e. what differentiates one object from the other) is represented by the fields:

    protected String name; // the name of the LocalDispatcher instance
    protected DispatchContext ctx; // the DispatchContext: there is always a one to one mapping between a LocalDispatcher and a DispatchContext
    protected ServiceDispatcher dispatcher; // a reference to the ServiceDispatcher object associated to the Delegator used in the call ServiceContainer.getLocalDispatcher(dispatcherName, delegator)

As you can see there is nothing special in this status; name and ctx don't do anything special (DispatchContext holds simply a reference to the LocalDispatcher and has some utils methods) apart from the reference to the ServiceDispatcher (several GenericDispatcher holds the reference to the same ServiceDispatcher, the one associated with the delegator).

Issues/topics to discuss:

* the implementation of dispatcherCache in ServiceContainer is wrong because it assumes that you can uniquely identify a LocalDispatcher/DispatchContext from the dispatcherName name only; which is wrong because you also need a delegator
* but the main question is: considering the layout described above, what is the purpose/goal of having several instances of LocalDispatcher with different names? Shouldn't we simply create one instance per delegator? at that point it will be very easy to discard the ServiceDispatcher and integrate its logic into the GenericDispatcher; then we will remove all the references to ServiceDispatcher and replace them with references to the interface LocalDipatcher

I hope it makes sense, thanks for reading all this stuff.

Regards,

Jacopo

[Jacopo Cappellato-4]

On Jul 26, 2012, at 11:51 AM, Jacopo Cappellato wrote:

> * but the main question is: considering the layout described above, what is the purpose/goal of having several instances of LocalDispatcher with different names? Shouldn't we simply create one instance per delegator?

As a side note, this change(after the recent refactoring) should be rather easy to implement; in fact it will be a matter of changing the signature of the

LocalDispatcher dispatcher = ServiceContainer.getLocalDispatcher(String dispatcherName, Delegator delegator);

into:

LocalDispatcher dispatcher = ServiceContainer.getLocalDispatcher(Delegator delegator);

and we will no more have to add the dispatcher name to the web.xml file of all the web applications, for example:

  <context-param>
    <param-name>localDispatcherName</param-name>
    <param-value>webtools</param-value>
    <description>A unique name used to identify/recognize the local dispatcher for the Service Engine</description>
  </context-param>

Kind regards,

Jacopo

[Adrian Crum-3]

I think this has something to do with each application being a separate
web application (in a J2EE sense), but I'm just guessing. There seems to
be a reason you would need some services local to (or restricted to) a
web application, but I don't know what the reason is.

-Adrian

On 7/26/2012 1:32 PM, Jacopo Cappellato wrote:

> On Jul 26, 2012, at 11:51 AM, Jacopo Cappellato wrote:
>
>> * but the main question is: considering the layout described above, what is the purpose/goal of having several instances of LocalDispatcher with different names? Shouldn't we simply create one instance per delegator?
> As a side note, this change(after the recent refactoring) should be rather easy to implement; in fact it will be a matter of changing the signature of the
>
> LocalDispatcher dispatcher = ServiceContainer.getLocalDispatcher(String dispatcherName, Delegator delegator);
>
> into:
>
> LocalDispatcher dispatcher = ServiceContainer.getLocalDispatcher(Delegator delegator);
>
> and we will no more have to add the dispatcher name to the web.xml file of all the web applications, for example:
>
>    <context-param>
>      <param-name>localDispatcherName</param-name>
>      <param-value>webtools</param-value>
>      <description>A unique name used to identify/recognize the local dispatcher for the Service Engine</description>
>    </context-param>
>
> Kind regards,
>
> Jacopo
>

[Jacques Le Roux]

I did not have the time to read all the thread... I find useful to be able to read the local dispatcher name from DispatchContext in
services to know from which webapp the service was called (webtools being a specific case, for instance when you run services
from there...)

HTH

Jacques

From: "Adrian Crum" <[hidden email]>

>I think this has something to do with each application being a separate web application (in a J2EE sense), but I'm just guessing.
>There seems to be a reason you would need some services local to (or restricted to) a web application, but I don't know what the
>reason is.
>
> -Adrian
>
> On 7/26/2012 1:32 PM, Jacopo Cappellato wrote:
>> On Jul 26, 2012, at 11:51 AM, Jacopo Cappellato wrote:
>>
>>> * but the main question is: considering the layout described above, what is the purpose/goal of having several instances of
>>> LocalDispatcher with different names? Shouldn't we simply create one instance per delegator?
>> As a side note, this change(after the recent refactoring) should be rather easy to implement; in fact it will be a matter of
>> changing the signature of the
>>
>> LocalDispatcher dispatcher = ServiceContainer.getLocalDispatcher(String dispatcherName, Delegator delegator);
>>
>> into:
>>
>> LocalDispatcher dispatcher = ServiceContainer.getLocalDispatcher(Delegator delegator);
>>
>> and we will no more have to add the dispatcher name to the web.xml file of all the web applications, for example:
>>
>>    <context-param>
>>      <param-name>localDispatcherName</param-name>
>>      <param-value>webtools</param-value>
>>      <description>A unique name used to identify/recognize the local dispatcher for the Service Engine</description>
>>    </context-param>
>>
>> Kind regards,
>>
>> Jacopo
>>
>

[Jacopo Cappellato-4]

On Jul 26, 2012, at 3:47 PM, Adrian Crum wrote:

> I think this has something to do with each application being a separate web application (in a J2EE sense), but I'm just guessing. There seems to be a reason you would need some services local to (or restricted to) a web application, but I don't know what the reason is.

This was actually an old feature that was available but never used in years, and I have excluded it in my recent refactoring (to make the code less complex, more efficient and thread safe); I guess that was part of an initial plan but then OFBiz evolved in a more component-centric rather than webapp-centric architecture and I doubt we will ever need that feature.

Jacopo

>
> -Adrian
>
> On 7/26/2012 1:32 PM, Jacopo Cappellato wrote:
>> On Jul 26, 2012, at 11:51 AM, Jacopo Cappellato wrote:
>>
>>> * but the main question is: considering the layout described above, what is the purpose/goal of having several instances of LocalDispatcher with different names? Shouldn't we simply create one instance per delegator?
>> As a side note, this change(after the recent refactoring) should be rather easy to implement; in fact it will be a matter of changing the signature of the
>>
>> LocalDispatcher dispatcher = ServiceContainer.getLocalDispatcher(String dispatcherName, Delegator delegator);
>>
>> into:
>>
>> LocalDispatcher dispatcher = ServiceContainer.getLocalDispatcher(Delegator delegator);
>>
>> and we will no more have to add the dispatcher name to the web.xml file of all the web applications, for example:
>>
>>   <context-param>
>>     <param-name>localDispatcherName</param-name>
>>     <param-value>webtools</param-value>
>>     <description>A unique name used to identify/recognize the local dispatcher for the Service Engine</description>
>>   </context-param>
>>
>> Kind regards,
>>
>> Jacopo
>>
>

[Adrian Crum-3]

As long as no one else has a problem with it, I agree that we should
eliminate the unused feature.

-Adrian

On 7/26/2012 5:25 PM, Jacopo Cappellato wrote:

> On Jul 26, 2012, at 3:47 PM, Adrian Crum wrote:
>
>> I think this has something to do with each application being a separate web application (in a J2EE sense), but I'm just guessing. There seems to be a reason you would need some services local to (or restricted to) a web application, but I don't know what the reason is.
> This was actually an old feature that was available but never used in years, and I have excluded it in my recent refactoring (to make the code less complex, more efficient and thread safe); I guess that was part of an initial plan but then OFBiz evolved in a more component-centric rather than webapp-centric architecture and I doubt we will ever need that feature.
>
> Jacopo
>
>> -Adrian
>>
>> On 7/26/2012 1:32 PM, Jacopo Cappellato wrote:
>>> On Jul 26, 2012, at 11:51 AM, Jacopo Cappellato wrote:
>>>
>>>> * but the main question is: considering the layout described above, what is the purpose/goal of having several instances of LocalDispatcher with different names? Shouldn't we simply create one instance per delegator?
>>> As a side note, this change(after the recent refactoring) should be rather easy to implement; in fact it will be a matter of changing the signature of the
>>>
>>> LocalDispatcher dispatcher = ServiceContainer.getLocalDispatcher(String dispatcherName, Delegator delegator);
>>>
>>> into:
>>>
>>> LocalDispatcher dispatcher = ServiceContainer.getLocalDispatcher(Delegator delegator);
>>>
>>> and we will no more have to add the dispatcher name to the web.xml file of all the web applications, for example:
>>>
>>>    <context-param>
>>>      <param-name>localDispatcherName</param-name>
>>>      <param-value>webtools</param-value>
>>>      <description>A unique name used to identify/recognize the local dispatcher for the Service Engine</description>
>>>    </context-param>
>>>
>>> Kind regards,
>>>
>>> Jacopo
>>>

[Adrian Crum-3]

That seems to be an odd requirement. What if the service call didn't
originate from a webapp?

-Adrian

On 7/26/2012 3:11 PM, Jacques Le Roux wrote:

> I did not have the time to read all the thread... I find useful to be
> able to read the local dispatcher name from DispatchContext in
> services to know from which webapp the service was called (webtools
> being a specific case, for instance when you run services
> from there...)
>
> HTH
>
> Jacques
>
> From: "Adrian Crum" <[hidden email]>
>> I think this has something to do with each application being a
>> separate web application (in a J2EE sense), but I'm just guessing.
>> There seems to be a reason you would need some services local to (or
>> restricted to) a web application, but I don't know what the
>> reason is.
>>
>> -Adrian
>>
>> On 7/26/2012 1:32 PM, Jacopo Cappellato wrote:
>>> On Jul 26, 2012, at 11:51 AM, Jacopo Cappellato wrote:
>>>
>>>> * but the main question is: considering the layout described above,
>>>> what is the purpose/goal of having several instances of
>>>> LocalDispatcher with different names? Shouldn't we simply create
>>>> one instance per delegator?
>>> As a side note, this change(after the recent refactoring) should be
>>> rather easy to implement; in fact it will be a matter of
>>> changing the signature of the
>>>
>>> LocalDispatcher dispatcher =
>>> ServiceContainer.getLocalDispatcher(String dispatcherName, Delegator
>>> delegator);
>>>
>>> into:
>>>
>>> LocalDispatcher dispatcher =
>>> ServiceContainer.getLocalDispatcher(Delegator delegator);
>>>
>>> and we will no more have to add the dispatcher name to the web.xml
>>> file of all the web applications, for example:
>>>
>>>    <context-param>
>>>      <param-name>localDispatcherName</param-name>
>>>      <param-value>webtools</param-value>
>>>      <description>A unique name used to identify/recognize the local
>>> dispatcher for the Service Engine</description>
>>>    </context-param>
>>>
>>> Kind regards,
>>>
>>> Jacopo
>>>
>>

[Jacques Le Roux]

The requirement is to be able to recognize in which webapp a service is called. If it's not called in the context of a webapp then
it's another problematic. Notably if it's called as a job, where then the webapp is always webtools. I will not consider this aspect
for now (then you need another way to know in which context the service is supposed to be called, but nevermind, not the issue at
hand). I just would like to be sure that inside a service, using DispatchContext.getName() you can still return the name of the
localDispatcher which is currently defined in the web.xml file of the webapp. Of course if another smarter mechanism is able to do
so, I'd see no problems. Else I would consider this a major regression and would really like to know what are the reasons behind,
apart refactoring satisfaction (I know this feeling)... When it's not broken don't fix it...

Jacques

From: "Adrian Crum" <[hidden email]>

> That seems to be an odd requirement. What if the service call didn't originate from a webapp?
>
> -Adrian
>
> On 7/26/2012 3:11 PM, Jacques Le Roux wrote:
>> I did not have the time to read all the thread... I find useful to be able to read the local dispatcher name from DispatchContext
>> in
>> services to know from which webapp the service was called (webtools being a specific case, for instance when you run services
>> from there...)
>>
>> HTH
>>
>> Jacques
>>
>> From: "Adrian Crum" <[hidden email]>
>>> I think this has something to do with each application being a separate web application (in a J2EE sense), but I'm just
>>> guessing.
>>> There seems to be a reason you would need some services local to (or restricted to) a web application, but I don't know what the
>>> reason is.
>>>
>>> -Adrian
>>>
>>> On 7/26/2012 1:32 PM, Jacopo Cappellato wrote:
>>>> On Jul 26, 2012, at 11:51 AM, Jacopo Cappellato wrote:
>>>>
>>>>> * but the main question is: considering the layout described above, what is the purpose/goal of having several instances of
>>>>> LocalDispatcher with different names? Shouldn't we simply create one instance per delegator?
>>>> As a side note, this change(after the recent refactoring) should be rather easy to implement; in fact it will be a matter of
>>>> changing the signature of the
>>>>
>>>> LocalDispatcher dispatcher = ServiceContainer.getLocalDispatcher(String dispatcherName, Delegator delegator);
>>>>
>>>> into:
>>>>
>>>> LocalDispatcher dispatcher = ServiceContainer.getLocalDispatcher(Delegator delegator);
>>>>
>>>> and we will no more have to add the dispatcher name to the web.xml file of all the web applications, for example:
>>>>
>>>>    <context-param>
>>>>      <param-name>localDispatcherName</param-name>
>>>>      <param-value>webtools</param-value>
>>>>      <description>A unique name used to identify/recognize the local dispatcher for the Service Engine</description>
>>>>    </context-param>
>>>>
>>>> Kind regards,
>>>>
>>>> Jacopo
>>>>
>>>
>

[Scott Gray-2]

No offense but it sounds to me like you're using a hack in place of a proper solution and are now concerned that it might go away?  Can you provide a use case example for where a service might need to know which webapp it was called from?  Service data is supposed to be supplied via the context, just because the dispatcher name happens to (but has never been required to) match the webapp name doesn't make it a good solution.

Regards
Scott

On 27/07/2012, at 8:27 AM, Jacques Le Roux wrote:

> The requirement is to be able to recognize in which webapp a service is called. If it's not called in the context of a webapp then it's another problematic. Notably if it's called as a job, where then the webapp is always webtools. I will not consider this aspect for now (then you need another way to know in which context the service is supposed to be called, but nevermind, not the issue at hand). I just would like to be sure that inside a service, using DispatchContext.getName() you can still return the name of the localDispatcher which is currently defined in the web.xml file of the webapp. Of course if another smarter mechanism is able to do so, I'd see no problems. Else I would consider this a major regression and would really like to know what are the reasons behind, apart refactoring satisfaction (I know this feeling)... When it's not broken don't fix it...
>
> Jacques
>
> From: "Adrian Crum" <[hidden email]>
>> That seems to be an odd requirement. What if the service call didn't originate from a webapp?
>>
>> -Adrian
>>
>> On 7/26/2012 3:11 PM, Jacques Le Roux wrote:
>>> I did not have the time to read all the thread... I find useful to be able to read the local dispatcher name from DispatchContext in
>>> services to know from which webapp the service was called (webtools being a specific case, for instance when you run services
>>> from there...)
>>>
>>> HTH
>>>
>>> Jacques
>>>
>>> From: "Adrian Crum" <[hidden email]>
>>>> I think this has something to do with each application being a separate web application (in a J2EE sense), but I'm just guessing.
>>>> There seems to be a reason you would need some services local to (or restricted to) a web application, but I don't know what the
>>>> reason is.
>>>>
>>>> -Adrian
>>>>
>>>> On 7/26/2012 1:32 PM, Jacopo Cappellato wrote:
>>>>> On Jul 26, 2012, at 11:51 AM, Jacopo Cappellato wrote:
>>>>>
>>>>>> * but the main question is: considering the layout described above, what is the purpose/goal of having several instances of
>>>>>> LocalDispatcher with different names? Shouldn't we simply create one instance per delegator?
>>>>> As a side note, this change(after the recent refactoring) should be rather easy to implement; in fact it will be a matter of
>>>>> changing the signature of the
>>>>>
>>>>> LocalDispatcher dispatcher = ServiceContainer.getLocalDispatcher(String dispatcherName, Delegator delegator);
>>>>>
>>>>> into:
>>>>>
>>>>> LocalDispatcher dispatcher = ServiceContainer.getLocalDispatcher(Delegator delegator);
>>>>>
>>>>> and we will no more have to add the dispatcher name to the web.xml file of all the web applications, for example:
>>>>>
>>>>>   <context-param>
>>>>>     <param-name>localDispatcherName</param-name>
>>>>>     <param-value>webtools</param-value>
>>>>>     <description>A unique name used to identify/recognize the local dispatcher for the Service Engine</description>
>>>>>   </context-param>
>>>>>
>>>>> Kind regards,
>>>>>
>>>>> Jacopo
>>>>>
>>>>

[Jacques Le Roux]

The local dispatcher name must not match the webapp name. It just has to exist in the web.xlm of the webapp.

Consider this use case, you have *many* webapps.  They all share a set of properties and the values of these properties differ by
webapp (hosts, urls, tokens, timeouts, boolean values, etc.). A simple mechanism is then to have a properties  file by webapp, named
after the local dispatcher name, and you have a very *pramatic* way (think production, properties files in version system agains
contents in DB for instance) to dynamically adapt those values in function of the webapp context. BTW, if you would pass this value
as a service data you would still need to know in which webapp you are running. How would you decide that when calling a service in
a service where you have no direct access to request or session? Of course you could still use ServletContext.getInitParameter(),
but then things get complicated, why complicate them since there is already a valid mechanism?

I don't care it it goes away for this *specific case* because it will be still easy for me to re-add it. Actually I'd then even
generalise to avoid to have to pass the local dispatcher name  to async called services called sync services in a webapp. I'd then
use the same mechanism than is used to automatically pass the default/current userlogin for instance.

I was just warning *everybody* that it might not be a great idea to remove a feature that's not broken. For which reasons BTW? Is it
blocking/breaking something?

Also did you read this comment
https://cwiki.apache.org/confluence/display/OFBTECH/Service+Engine+Guide#ServiceEngineGuide-ServiceDispatcher ?
<<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.>>

Hope I clarified

Jacques

From: "Scott Gray" <[hidden email]>

> No offense but it sounds to me like you're using a hack in place of a proper solution and are now concerned that it might go away?
> Can you provide a use case example for where a service might need to know which webapp it was called from?  Service data is
> supposed to be supplied via the context, just because the dispatcher name happens to (but has never been required to) match the
> webapp name doesn't make it a good solution.
>
> Regards
> Scott
>
> On 27/07/2012, at 8:27 AM, Jacques Le Roux wrote:
>
>> The requirement is to be able to recognize in which webapp a service is called. If it's not called in the context of a webapp
>> then it's another problematic. Notably if it's called as a job, where then the webapp is always webtools. I will not consider
>> this aspect for now (then you need another way to know in which context the service is supposed to be called, but nevermind, not
>> the issue at hand). I just would like to be sure that inside a service, using DispatchContext.getName() you can still return the
>> name of the localDispatcher which is currently defined in the web.xml file of the webapp. Of course if another smarter mechanism
>> is able to do so, I'd see no problems. Else I would consider this a major regression and would really like to know what are the
>> reasons behind, apart refactoring satisfaction (I know this feeling)... When it's not broken don't fix it...
>>
>> Jacques
>>
>> From: "Adrian Crum" <[hidden email]>
>>> That seems to be an odd requirement. What if the service call didn't originate from a webapp?
>>>
>>> -Adrian
>>>
>>> On 7/26/2012 3:11 PM, Jacques Le Roux wrote:
>>>> I did not have the time to read all the thread... I find useful to be able to read the local dispatcher name from
>>>> DispatchContext in
>>>> services to know from which webapp the service was called (webtools being a specific case, for instance when you run services
>>>> from there...)
>>>>
>>>> HTH
>>>>
>>>> Jacques
>>>>
>>>> From: "Adrian Crum" <[hidden email]>
>>>>> I think this has something to do with each application being a separate web application (in a J2EE sense), but I'm just
>>>>> guessing.
>>>>> There seems to be a reason you would need some services local to (or restricted to) a web application, but I don't know what
>>>>> the
>>>>> reason is.
>>>>>
>>>>> -Adrian
>>>>>
>>>>> On 7/26/2012 1:32 PM, Jacopo Cappellato wrote:
>>>>>> On Jul 26, 2012, at 11:51 AM, Jacopo Cappellato wrote:
>>>>>>
>>>>>>> * but the main question is: considering the layout described above, what is the purpose/goal of having several instances of
>>>>>>> LocalDispatcher with different names? Shouldn't we simply create one instance per delegator?
>>>>>> As a side note, this change(after the recent refactoring) should be rather easy to implement; in fact it will be a matter of
>>>>>> changing the signature of the
>>>>>>
>>>>>> LocalDispatcher dispatcher = ServiceContainer.getLocalDispatcher(String dispatcherName, Delegator delegator);
>>>>>>
>>>>>> into:
>>>>>>
>>>>>> LocalDispatcher dispatcher = ServiceContainer.getLocalDispatcher(Delegator delegator);
>>>>>>
>>>>>> and we will no more have to add the dispatcher name to the web.xml file of all the web applications, for example:
>>>>>>
>>>>>>   <context-param>
>>>>>>     <param-name>localDispatcherName</param-name>
>>>>>>     <param-value>webtools</param-value>
>>>>>>     <description>A unique name used to identify/recognize the local dispatcher for the Service Engine</description>
>>>>>>   </context-param>
>>>>>>
>>>>>> Kind regards,
>>>>>>
>>>>>> Jacopo
>>>>>>
>>>>>
>
>

[Jacques Le Roux]

From: "Jacques Le Roux" <[hidden email]>

> The local dispatcher name must not match the webapp name. It just has to exist in the web.xlm of the webapp.
>
> Consider this use case, you have *many* webapps.  They all share a set of properties and the values of these properties differ by
> webapp (hosts, urls, tokens, timeouts, boolean values, etc.). A simple mechanism is then to have a properties  file by webapp,
> named after the local dispatcher name, and you have a very *pramatic* way (think production, properties files in version system
> agains contents in DB for instance) to dynamically adapt those values in function of the webapp context. BTW, if you would pass
> this value as a service data you would still need to know in which webapp you are running. How would you decide that when calling
> a service in a service where you have no direct access to request or session? Of course you could still use
> ServletContext.getInitParameter(), but then things get complicated, why complicate them since there is already a valid mechanism?
>
> I don't care it it goes away for this *specific case* because it will be still easy for me to re-add it. Actually I'd then even
> generalise to avoid to have to pass the local dispatcher name  to async called services called sync services in a webapp. I'd then
> use the same mechanism than is used to automatically pass the default/current userlogin for instance.

Typo:
generalise to avoid to have to pass the local dispatcher name  to async called services called __FROM__ sync services in a webapp

> I was just warning *everybody* that it might not be a great idea to remove a feature that's not broken. For which reasons BTW? Is
> it blocking/breaking something?
>
> Also did you read this comment
> https://cwiki.apache.org/confluence/display/OFBTECH/Service+Engine+Guide#ServiceEngineGuide-ServiceDispatcher ?
> <<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.>>
>
> Hope I clarified
>
> Jacques
>
> From: "Scott Gray" <[hidden email]>
>> No offense but it sounds to me like you're using a hack in place of a proper solution and are now concerned that it might go
>> away? Can you provide a use case example for where a service might need to know which webapp it was called from?  Service data is
>> supposed to be supplied via the context, just because the dispatcher name happens to (but has never been required to) match the
>> webapp name doesn't make it a good solution.
>>
>> Regards
>> Scott
>>
>> On 27/07/2012, at 8:27 AM, Jacques Le Roux wrote:
>>
>>> The requirement is to be able to recognize in which webapp a service is called. If it's not called in the context of a webapp
>>> then it's another problematic. Notably if it's called as a job, where then the webapp is always webtools. I will not consider
>>> this aspect for now (then you need another way to know in which context the service is supposed to be called, but nevermind, not
>>> the issue at hand). I just would like to be sure that inside a service, using DispatchContext.getName() you can still return the
>>> name of the localDispatcher which is currently defined in the web.xml file of the webapp. Of course if another smarter mechanism
>>> is able to do so, I'd see no problems. Else I would consider this a major regression and would really like to know what are the
>>> reasons behind, apart refactoring satisfaction (I know this feeling)... When it's not broken don't fix it...
>>>
>>> Jacques
>>>
>>> From: "Adrian Crum" <[hidden email]>
>>>> That seems to be an odd requirement. What if the service call didn't originate from a webapp?
>>>>
>>>> -Adrian
>>>>
>>>> On 7/26/2012 3:11 PM, Jacques Le Roux wrote:
>>>>> I did not have the time to read all the thread... I find useful to be able to read the local dispatcher name from
>>>>> DispatchContext in
>>>>> services to know from which webapp the service was called (webtools being a specific case, for instance when you run services
>>>>> from there...)
>>>>>
>>>>> HTH
>>>>>
>>>>> Jacques
>>>>>
>>>>> From: "Adrian Crum" <[hidden email]>
>>>>>> I think this has something to do with each application being a separate web application (in a J2EE sense), but I'm just
>>>>>> guessing.
>>>>>> There seems to be a reason you would need some services local to (or restricted to) a web application, but I don't know what
>>>>>> the
>>>>>> reason is.
>>>>>>
>>>>>> -Adrian
>>>>>>
>>>>>> On 7/26/2012 1:32 PM, Jacopo Cappellato wrote:
>>>>>>> On Jul 26, 2012, at 11:51 AM, Jacopo Cappellato wrote:
>>>>>>>
>>>>>>>> * but the main question is: considering the layout described above, what is the purpose/goal of having several instances of
>>>>>>>> LocalDispatcher with different names? Shouldn't we simply create one instance per delegator?
>>>>>>> As a side note, this change(after the recent refactoring) should be rather easy to implement; in fact it will be a matter of
>>>>>>> changing the signature of the
>>>>>>>
>>>>>>> LocalDispatcher dispatcher = ServiceContainer.getLocalDispatcher(String dispatcherName, Delegator delegator);
>>>>>>>
>>>>>>> into:
>>>>>>>
>>>>>>> LocalDispatcher dispatcher = ServiceContainer.getLocalDispatcher(Delegator delegator);
>>>>>>>
>>>>>>> and we will no more have to add the dispatcher name to the web.xml file of all the web applications, for example:
>>>>>>>
>>>>>>>   <context-param>
>>>>>>>     <param-name>localDispatcherName</param-name>
>>>>>>>     <param-value>webtools</param-value>
>>>>>>>     <description>A unique name used to identify/recognize the local dispatcher for the Service Engine</description>
>>>>>>>   </context-param>
>>>>>>>
>>>>>>> Kind regards,
>>>>>>>
>>>>>>> Jacopo
>>>>>>>
>>>>>>
>>
>>

[Jacopo Cappellato-4]

On Jul 27, 2012, at 7:14 AM, Jacques Le Roux wrote:

> I was just warning *everybody* that it might not be a great idea to remove a feature that's not broken. For which reasons BTW? Is it blocking/breaking something?

The current mechanism is actually broken (i.e. implemented in a bad way) but its effect are not evident because they don't manifest with the ootb configuration; for example a use case like this would break the system:

1) define two delegators associated to different model readers:

    <delegator name="default" entity-model-reader="main" .../>
    <delegator name="second" entity-model-reader="alternative" .../>

2) now if you call:

LocalDispatcher facility = ServiceContainer.getLocalDispatcher("facility", delegator);
LocalDispatcher alternativeFacility = ServiceContainer.getLocalDispatcher("facility", alternativeDelegator);

you will actually get the same object; i.e. the second call will get from the cache the object created by the first call, i.e. a dispatcher associated to the wrong delegator/model (delegator instead of alternativeDelegator).

Regards,

Jacopo

[Jacopo Cappellato-4]

On Jul 27, 2012, at 7:14 AM, Jacques Le Roux wrote:

> Also did you read this comment https://cwiki.apache.org/confluence/display/OFBTECH/Service+Engine+Guide#ServiceEngineGuide-ServiceDispatcher ?

Thank you Jacques, the document is really interesting because it describes the early design and confirms my understandings after the research/review/refactoring I recently did, which brought up the questions/proposals we are discussing now.
I would like to mention that my refactoring simplified greatly the implementation (that was a bit messed up, as it is rather natural, after years of modifications) but it didn't change the concepts described there... it actually makes the code closer to them because it is easier to read it. However, as part of the refactoring, I also got rid of a few old and never used features that are still documented in the document; for example:

* "The DispatchContext [...] contains [...] a 'bag' of user defined attributes."

this feature is no more available.

In general there are other details about it that are no more accurate (not because of my refactoring); for example it is not true that the DispatchContext holds a reference to a delegator

Should we update the document to reflect the recent changes?

Kind regards,

Jacopo

[Scott Gray-2]

Seems like webSiteId would be the best parameter to pass into the service:
- It would be populated automatically for services executed from request events
- WebSite already stores URLs and other site specific data
- Services being called from services where the parent service isn't carrying the data required should really be refactored. Service groups are also a useful mechanism for chained services where all services share a common context but only take what they need.

Also, database configuration is always preferable over properties files IMO.

Regards
Scott

On 27/07/2012, at 5:14 PM, Jacques Le Roux wrote:

> The local dispatcher name must not match the webapp name. It just has to exist in the web.xlm of the webapp.
>
> Consider this use case, you have *many* webapps.  They all share a set of properties and the values of these properties differ by webapp (hosts, urls, tokens, timeouts, boolean values, etc.). A simple mechanism is then to have a properties  file by webapp, named after the local dispatcher name, and you have a very *pramatic* way (think production, properties files in version system agains contents in DB for instance) to dynamically adapt those values in function of the webapp context. BTW, if you would pass this value as a service data you would still need to know in which webapp you are running. How would you decide that when calling a service in a service where you have no direct access to request or session? Of course you could still use ServletContext.getInitParameter(), but then things get complicated, why complicate them since there is already a valid mechanism?
>
> I don't care it it goes away for this *specific case* because it will be still easy for me to re-add it. Actually I'd then even generalise to avoid to have to pass the local dispatcher name  to async called services called sync services in a webapp. I'd then use the same mechanism than is used to automatically pass the default/current userlogin for instance.
>
> I was just warning *everybody* that it might not be a great idea to remove a feature that's not broken. For which reasons BTW? Is it blocking/breaking something?
>
> Also did you read this comment https://cwiki.apache.org/confluence/display/OFBTECH/Service+Engine+Guide#ServiceEngineGuide-ServiceDispatcher ?
> <<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.>>
>
> Hope I clarified
>
> Jacques
>
> From: "Scott Gray" <[hidden email]>
>> No offense but it sounds to me like you're using a hack in place of a proper solution and are now concerned that it might go away? Can you provide a use case example for where a service might need to know which webapp it was called from?  Service data is supposed to be supplied via the context, just because the dispatcher name happens to (but has never been required to) match the webapp name doesn't make it a good solution.
>>
>> Regards
>> Scott
>>
>> On 27/07/2012, at 8:27 AM, Jacques Le Roux wrote:
>>
>>> The requirement is to be able to recognize in which webapp a service is called. If it's not called in the context of a webapp then it's another problematic. Notably if it's called as a job, where then the webapp is always webtools. I will not consider this aspect for now (then you need another way to know in which context the service is supposed to be called, but nevermind, not the issue at hand). I just would like to be sure that inside a service, using DispatchContext.getName() you can still return the name of the localDispatcher which is currently defined in the web.xml file of the webapp. Of course if another smarter mechanism is able to do so, I'd see no problems. Else I would consider this a major regression and would really like to know what are the reasons behind, apart refactoring satisfaction (I know this feeling)... When it's not broken don't fix it...
>>>
>>> Jacques
>>>
>>> From: "Adrian Crum" <[hidden email]>
>>>> That seems to be an odd requirement. What if the service call didn't originate from a webapp?
>>>>
>>>> -Adrian
>>>>
>>>> On 7/26/2012 3:11 PM, Jacques Le Roux wrote:
>>>>> I did not have the time to read all the thread... I find useful to be able to read the local dispatcher name from DispatchContext in
>>>>> services to know from which webapp the service was called (webtools being a specific case, for instance when you run services
>>>>> from there...)
>>>>>
>>>>> HTH
>>>>>
>>>>> Jacques
>>>>>
>>>>> From: "Adrian Crum" <[hidden email]>
>>>>>> I think this has something to do with each application being a separate web application (in a J2EE sense), but I'm just guessing.
>>>>>> There seems to be a reason you would need some services local to (or restricted to) a web application, but I don't know what the
>>>>>> reason is.
>>>>>>
>>>>>> -Adrian
>>>>>>
>>>>>> On 7/26/2012 1:32 PM, Jacopo Cappellato wrote:
>>>>>>> On Jul 26, 2012, at 11:51 AM, Jacopo Cappellato wrote:
>>>>>>>
>>>>>>>> * but the main question is: considering the layout described above, what is the purpose/goal of having several instances of
>>>>>>>> LocalDispatcher with different names? Shouldn't we simply create one instance per delegator?
>>>>>>> As a side note, this change(after the recent refactoring) should be rather easy to implement; in fact it will be a matter of
>>>>>>> changing the signature of the
>>>>>>>
>>>>>>> LocalDispatcher dispatcher = ServiceContainer.getLocalDispatcher(String dispatcherName, Delegator delegator);
>>>>>>>
>>>>>>> into:
>>>>>>>
>>>>>>> LocalDispatcher dispatcher = ServiceContainer.getLocalDispatcher(Delegator delegator);
>>>>>>>
>>>>>>> and we will no more have to add the dispatcher name to the web.xml file of all the web applications, for example:
>>>>>>>
>>>>>>>  <context-param>
>>>>>>>    <param-name>localDispatcherName</param-name>
>>>>>>>    <param-value>webtools</param-value>
>>>>>>>    <description>A unique name used to identify/recognize the local dispatcher for the Service Engine</description>
>>>>>>>  </context-param>
>>>>>>>
>>>>>>> Kind regards,
>>>>>>>
>>>>>>> Jacopo
>>>>>>>
>>>>>>
>>

[Jacques Le Roux]

Thanks for clarification Jacopo,

This indeed needs to be fixed then. Does this mean that the DispatchContext name attribute and its getter must be removed?

Jacques

From: "Jacopo Cappellato" <[hidden email]>

> On Jul 27, 2012, at 7:14 AM, Jacques Le Roux wrote:
>
>> I was just warning *everybody* that it might not be a great idea to remove a feature that's not broken. For which reasons BTW? Is
>> it blocking/breaking something?
>
> The current mechanism is actually broken (i.e. implemented in a bad way) but its effect are not evident because they don't
> manifest with the ootb configuration; for example a use case like this would break the system:
>
> 1) define two delegators associated to different model readers:
>
>    <delegator name="default" entity-model-reader="main" .../>
>    <delegator name="second" entity-model-reader="alternative" .../>
>
> 2) now if you call:
>
> LocalDispatcher facility = ServiceContainer.getLocalDispatcher("facility", delegator);
> LocalDispatcher alternativeFacility = ServiceContainer.getLocalDispatcher("facility", alternativeDelegator);
>
> you will actually get the same object; i.e. the second call will get from the cache the object created by the first call, i.e. a
> dispatcher associated to the wrong delegator/model (delegator instead of alternativeDelegator).
>
> Regards,
>
> Jacopo

[Jacques Le Roux]

From: "Jacopo Cappellato" <[hidden email]>

> On Jul 27, 2012, at 7:14 AM, Jacques Le Roux wrote:
>
>> Also did you read this comment
>> https://cwiki.apache.org/confluence/display/OFBTECH/Service+Engine+Guide#ServiceEngineGuide-ServiceDispatcher ?
>
> Thank you Jacques, the document is really interesting because it describes the early design and confirms my understandings after
> the research/review/refactoring I recently did, which brought up the questions/proposals we are discussing now.
> I would like to mention that my refactoring simplified greatly the implementation (that was a bit messed up, as it is rather
> natural, after years of modifications) but it didn't change the concepts described there... it actually makes the code closer to
> them because it is easier to read it. However, as part of the refactoring, I also got rid of a few old and never used features
> that are still documented in the document; for example:
>
> * "The DispatchContext [...] contains [...] a 'bag' of user defined attributes."
>
> this feature is no more available.
>
> In general there are other details about it that are no more accurate (not because of my refactoring); for example it is not true
> that the DispatchContext holds a reference to a delegator

Will they not miss to anyone? What is supposed to replace them? Were they really useless?

> Should we update the document to reflect the recent changes?

Sure that's is most needed IMO (should not even be questionned ;o)

Jacques

>
> Kind regards,
>
> Jacopo

[Jacques Le Roux]

From: "Scott Gray" <[hidden email]>
> Seems like webSiteId would be the best parameter to pass into the service:
> - It would be populated automatically for services executed from request events
> - WebSite already stores URLs and other site specific data
> - Services being called from services where the parent service isn't carrying the data required should really be refactored.
> Service groups are also a useful mechanism for chained services where all services share a common context but only take what they
> need.

webSiteId could be an alternative but you would need to pass it in all cases. This means to retrieve it before, etc.
By using DispatchContext.getName() you don't have to pass anything in most cases. The local dispatcher name is already there and
accurate but for async calls (I let apart web service and such which are another subject). Why remove it (maybe the answer came from
last Jacopo mails)?
Actually I must say I got this design from a non seasoned OFBiz team  and had to cope with it. I found their idea interesting
though incomplete (async calls). That's why I wanted to discuss it.

Website could be extended for more specific data. In the specific case I faced, I'd still prefer the properties file though. It's
pretty handy when you have a lot of of  data to handle in different areas (dev, staging, qa, prod) and they don't change much when
done. It's easier to replace a properties  file in a svn repo than to handle website seed data patches and/or DB accesses. But of
course DB side has also it advantages, no restart mostly.

For your last point, yes those are theorical best practices indeed. The point here was to not have to pass anything but rely on
DispatchContext.getName()
Also sometimes you don't get the budget to refactor, must keep the pace, face up what is already in place and novelties that come
everyday...

> Also, database configuration is always preferable over properties files IMO.

Depends, can be very tedious with *a lot of* webapps (hence *lot of* websites for instance) and different areas to configure. Then
you don't have the same capabilities/power than using your IDE and SVN. Another example: compare contents in DB and in a files repo
(history, comparaison, etc.), ok not really config.

Thanks for sharing your thoughts

Jacques

>
> Regards
> Scott
>
> On 27/07/2012, at 5:14 PM, Jacques Le Roux wrote:
>
>> The local dispatcher name must not match the webapp name. It just has to exist in the web.xlm of the webapp.
>>
>> Consider this use case, you have *many* webapps.  They all share a set of properties and the values of these properties differ by
>> webapp (hosts, urls, tokens, timeouts, boolean values, etc.). A simple mechanism is then to have a properties  file by webapp,
>> named after the local dispatcher name, and you have a very *pramatic* way (think production, properties files in version system
>> agains contents in DB for instance) to dynamically adapt those values in function of the webapp context. BTW, if you would pass
>> this value as a service data you would still need to know in which webapp you are running. How would you decide that when calling
>> a service in a service where you have no direct access to request or session? Of course you could still use
>> ServletContext.getInitParameter(), but then things get complicated, why complicate them since there is already a valid mechanism?
>>
>> I don't care it it goes away for this *specific case* because it will be still easy for me to re-add it. Actually I'd then even
>> generalise to avoid to have to pass the local dispatcher name  to async called services called sync services in a webapp. I'd
>> then use the same mechanism than is used to automatically pass the default/current userlogin for instance.
>>
>> I was just warning *everybody* that it might not be a great idea to remove a feature that's not broken. For which reasons BTW? Is
>> it blocking/breaking something?
>>
>> Also did you read this comment
>> https://cwiki.apache.org/confluence/display/OFBTECH/Service+Engine+Guide#ServiceEngineGuide-ServiceDispatcher ?
>> <<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.>>
>>
>> Hope I clarified
>>
>> Jacques
>>
>> From: "Scott Gray" <[hidden email]>
>>> No offense but it sounds to me like you're using a hack in place of a proper solution and are now concerned that it might go
>>> away? Can you provide a use case example for where a service might need to know which webapp it was called from?  Service data
>>> is supposed to be supplied via the context, just because the dispatcher name happens to (but has never been required to) match
>>> the webapp name doesn't make it a good solution.
>>>
>>> Regards
>>> Scott
>>>
>>> On 27/07/2012, at 8:27 AM, Jacques Le Roux wrote:
>>>
>>>> The requirement is to be able to recognize in which webapp a service is called. If it's not called in the context of a webapp
>>>> then it's another problematic. Notably if it's called as a job, where then the webapp is always webtools. I will not consider
>>>> this aspect for now (then you need another way to know in which context the service is supposed to be called, but nevermind,
>>>> not the issue at hand). I just would like to be sure that inside a service, using DispatchContext.getName() you can still
>>>> return the name of the localDispatcher which is currently defined in the web.xml file of the webapp. Of course if another
>>>> smarter mechanism is able to do so, I'd see no problems. Else I would consider this a major regression and would really like to
>>>> know what are the reasons behind, apart refactoring satisfaction (I know this feeling)... When it's not broken don't fix it...
>>>>
>>>> Jacques
>>>>
>>>> From: "Adrian Crum" <[hidden email]>
>>>>> That seems to be an odd requirement. What if the service call didn't originate from a webapp?
>>>>>
>>>>> -Adrian
>>>>>
>>>>> On 7/26/2012 3:11 PM, Jacques Le Roux wrote:
>>>>>> I did not have the time to read all the thread... I find useful to be able to read the local dispatcher name from
>>>>>> DispatchContext in
>>>>>> services to know from which webapp the service was called (webtools being a specific case, for instance when you run services
>>>>>> from there...)
>>>>>>
>>>>>> HTH
>>>>>>
>>>>>> Jacques
>>>>>>
>>>>>> From: "Adrian Crum" <[hidden email]>
>>>>>>> I think this has something to do with each application being a separate web application (in a J2EE sense), but I'm just
>>>>>>> guessing.
>>>>>>> There seems to be a reason you would need some services local to (or restricted to) a web application, but I don't know what
>>>>>>> the
>>>>>>> reason is.
>>>>>>>
>>>>>>> -Adrian
>>>>>>>
>>>>>>> On 7/26/2012 1:32 PM, Jacopo Cappellato wrote:
>>>>>>>> On Jul 26, 2012, at 11:51 AM, Jacopo Cappellato wrote:
>>>>>>>>
>>>>>>>>> * but the main question is: considering the layout described above, what is the purpose/goal of having several instances
>>>>>>>>> of
>>>>>>>>> LocalDispatcher with different names? Shouldn't we simply create one instance per delegator?
>>>>>>>> As a side note, this change(after the recent refactoring) should be rather easy to implement; in fact it will be a matter
>>>>>>>> of
>>>>>>>> changing the signature of the
>>>>>>>>
>>>>>>>> LocalDispatcher dispatcher = ServiceContainer.getLocalDispatcher(String dispatcherName, Delegator delegator);
>>>>>>>>
>>>>>>>> into:
>>>>>>>>
>>>>>>>> LocalDispatcher dispatcher = ServiceContainer.getLocalDispatcher(Delegator delegator);
>>>>>>>>
>>>>>>>> and we will no more have to add the dispatcher name to the web.xml file of all the web applications, for example:
>>>>>>>>
>>>>>>>>  <context-param>
>>>>>>>>    <param-name>localDispatcherName</param-name>
>>>>>>>>    <param-value>webtools</param-value>
>>>>>>>>    <description>A unique name used to identify/recognize the local dispatcher for the Service Engine</description>
>>>>>>>>  </context-param>
>>>>>>>>
>>>>>>>> Kind regards,
>>>>>>>>
>>>>>>>> Jacopo
>>>>>>>>
>>>>>>>
>>>
>
>