Hi, Please see my comments inline.
On 05/22/2013 02:01 AM, Alessio Soldano wrote:
Hi Jim,
CC-ing ws dev list, as this is possibly of wider interest.
On 05/21/2013 09:29 AM, Jim Ma wrote:
> Hi Alessio,
>
> As we discussed yesterday, these are my two proposals we might can think
> about and refactor a bit in this or future release after complete
> jax-rpc removal.
>
> 1) Eliminate the EndpointRegistry, use EndpointService to get Endpoint info
> ATM, each deployed endpoint will be created with an EndpointService,
> and each EndpointService will be also registered into serviceContainer.
> With MSC's getServiceNames() and getService() we can get all the
> deployed endpoint information. ServiceContainer is a registry , so we
> don't need
> create our own registry to maintain the endpoint map.
> To resolve the stack/container depends issue , we can create similar
> spi api and factory class in integration layer, and return the endpoint
> list to cxf stack .
I've been thinking a bit about this proposal; the way I would possibly
see it implemented is in having a new implementation of the existing
EndpointRegistry SPI interface that internally relies on the container
services (through ServiceContainer). That would be created in the
existing EndpointRegistryService instead of the default impl that's
created now (we'd also need a managed version of it btw, implementing
ManagedEndpointRegistry).
There's no need for a new abstraction interface, given we already have
the EndpointRegistry one (the interface is fine and keeping it would
allow us to avoid touching the stack).
Agree with delegate EndpointRegistry to
ServiceContainer to get the
Endpoint .
If we did this new impl, we'd not need to explicitly
register/unregister
endpoints in the registry in start/stop methods of EndpointService. So
far so good.
Yes. This will bring us two pros: 1) as you said , no need to
register/unregister endpoints.
2) we can also simplify or remove the EndpointLifeCycleHandler: we
do this job when the
EndpointService is started/stopped.
The problem is that in change of saving the memory of a map
instance,
with the new impl we need to process the whole set of AS services each
time and filter out the WS endpoints ones through name matching, which
is possibly not very good performance-wise.
This is the cons of change we can
conclude. But at the moment our
EndpointRegistry is used in
these two places : WSEndpointMetrics and ServletHelper. I looked at
the code and
it looks it can be all refactored to get the Endpoint with the
ServiceContainer.getService(serivceName)
if we name the ServiceName with ContextRoot and endpointName without
deployment archive name.
The as7 console retrieves the endpoint info from the ModelNode which
is created
in the deployment phase, and it won't need call the serviceConintainer
and filter out the WS endpoint.
That said, unless we have other requirement to retrieve all the ws
endpoints from the serviceContainer,
it seems won't be a problem at the current deployment and management
architecture.
Moreover, we actually lose
control over the actual time an endpoint is registered/unregistered and
is hence available/unavailable for invocation.
So we have pros and cons, I'd say this needs to be properly evaluated...
IMHO, the state of EndpointService should be represented the state of
the Endpoint.
We don't need to ask the consumer to check the state of Endpoint.With
this concept,
we can evaluate how can we place the order of
EndpointServiceDeploymentAspect and
make it reflect the real state of an endpoint is ready for invocation.
> 2) Consolidate the jbossws-cxf and cxf management
> We now have ManagedEndpoint MBean to expose the management info to
> monitor the processTime, requestCount etc. If the cxf management is
> enabled in jboss-webservices.xml,
> some of the info are duplicate. We'd think about how to consolidate
> these two kind of management bean. Enable the cxf management by default
> is the way to go ?
The problem I see now is that cxf management is jmx / managed bean
based, and I think I remember the AS might run without the mbean server on.
To be better analysed, also considering that the stats needs to be
available through the Endpoint (they're exposed in the AS managemen, see
org.jboss.as.webservices.dmr.WSEndpointMetrics).
The problem I see here is if
cxf's management is enabled we exposed
duplicate
manament info though JMX. And customer will probably get the different
value
from CXF's JMX Bean and our JMX bean exposed from WSEndpointMetrics
although
it represents the same thing (for example our ProssTiime in a certain
Endpoint and
CXF's response time). Whether enrich our management and finally remove the
cxf's enabled flag or we make our management internally use the cxf's
management stuff,
it will be OK as long as we provide the unified interface to user. Just
my 2 cents.
This said, speaking of cleanups, there's actually something I thought
about I'd love to simply and just remembered while looking at your
registry proposal. That's the jbossws SPI resolution mechanism.
Currently we have a double resolution, so e.g. to get Foo we do:
SPIProvider spiProvider = SPIProviderResolver.getInstance().getProvider();
Foo foo = spiProvider.getSPI(Foo.class);
or (when a specific classloader is to be used):
SPIProvider spiProvider =
SPIProviderResolver.getInstance(classLoader).getProvider();
Foo foo = spiProvider.getSPI(Foo.class, classLoader);
both SPIProviderResolver#getInstance and SPIProvider#getSPI internally
rely on the jbossws ServiceLoader, which goes through the usual 3
resolution mechanism (service loader, propfile and sysprop). Moreover,
the default SPIResolver impl in jbws-common (which is what is actually
always used) allows using default values if nothing is found.
I'd like to simplify this; we could remove the SPIProviderResolver
indirection level by having a singleton in SPIProvider returned by a
static getInstance() and resolved once using the jbossws-spi
classloader. No need to try resolve the resolver each time; no need for
such a verbose api.
Something like:
public abstract class SPIProvider
{
private static SPIProvider me;
public static SPIProvider getInstance() {
if (me == null) {
me = SPIProviderResolver.getInstance().getProvider();
}
return me;
}
public <T> T getSPI(Class<T> spiType)
{
return getSPI(spiType, SecurityActions.getContextClassLoader());
}
public abstract <T> T getSPI(Class<T> spiType, ClassLoader loader);
}
and the use it as follows:
Foo foo = SPIProvider.getInstance().getSPI(Foo.class, classLoader);
I totally agree with this suggestion. It's very nice.
WDYT?
Cheers
Alessio