ECF / JaxRSProviders

Remote Services distribution provider based upon JaxRS. Includes imples based upon Jersey and CXF.
Apache License 2.0
13 stars 18 forks source link

HttpServiceHolder component usage #12

Closed fipro78 closed 5 years ago

fipro78 commented 5 years ago

Hi,

while trying to understand remote services with JAX-RS better, I looked through the code and came across the HttpServiceHolder. The implementation looks strange from a DS perspective, but I am not sure.

It is defined as a DS Component and it looks like it is intended to provide the HttpService. Why is that necessary? If DS is used there should be no need for such a class. If someone needs the HttpService he could inject it directly without the use of a HttpServiceHolder. If the intention is to also support non-DS environments, than still the implementation looks strange. In its current implementation it works in both DS and non-DS. But the DS benefits are not used in any way?

Maybe I am missing something, but like I said, from a DS perspective the implementation looks incorrect. I just wanted to inform about that, not sure where and in which scenarios this class is used.

Greez, Dirk

scottslewis commented 5 years ago

Hi Dirk,

Thanks for the issue and comments. For context: prior to ECF Photon we/I was trying to allow for/support usage of ECF's RS/RSA impl without SCR. This code started from that era.

More context: In supporting non-DS startup...for a service registry extension like RSA...JaxRS provider impls (CXF and Jersey currently) extended RSA with new distribution provider(s) by adding distribution provider in Activator. In the non-DS world this was necessary so that remote services exported upon startup would have their desired distribution provider (e.g. jaxrs.jersey available.

Again in that non-DS world, I wanted to make it easier for distribution provider implementers (other than me) to register servlets with httpservice(s). For example, org.eclipse.ecf.provider.jersey.server.JerseyServerContainer.createServlet only has to be implemented to setup the appropriate Servlet instance (jersey's ServletContainer class). A superclass can call createServlet, register with a set of properties, etc, etc.. rather than requiring the distribution provider to lookup the httpservice (again no DS), create and register their own servlet, setup and use servlet context, etc. (this was before httpservice whiteboard spec as well).

In any event, you are correct that...assuming DS...this can/could be simplified. I'm happy to use this issue as incentive for doing so in near future. It does work as desired now...with DS...i.e. it's not incorrect...but it does some things that are no longer necessary.

I think there is a general problem here though: When providing a mechanism for extending an extension (i.e. RSA is a service registry extension and ECF's impl can be extended with new distribution providers...which depend upon HttpService), startup timing (i.e. exporting a remote service) can be complicated.

Thanks for the comments and incentive to clean up.

scottslewis commented 5 years ago

I've fixed this as part of fix for issue 15. Removed HttpServiceHolder and service interface completely. Thanks.