Hi there, what if we will generate proxy that collects service's metrics only if service will implement some special interface? In such case:
- we won't affect existing services at all. - we can impose additional requirement for services that want use metrics out of box (i.e. service that implements our special interface *must* also have own interface and only invocations of methods of this interface will be taken into account for metrics collection). - user always can use own metrics framework instead of our (just do not implement this new special interface). About metrics enabling/disabling. At present IGNITE-11927 doesn't solve this problem. Just because there is no metrics implementation for services :) Anyway we should provide a way for configuring service metrics (in sense of enabled/disabled) during service deploy. It's easy for cases where deploy() methods have ServiceConfiguration as parameter. But there are "short cut" methods like deployXxxSingleton(). I have ideas how to solve this problem. For example we can introduce "short cut" factory methods like nodeSingletonConfiguration(String name, Service service) and clusterSingletonConfiguration(String name, Service service). This methods will return configuration which has parameters for given type of deployment and could be modified, e.g. metrics could be enabled. WDYT? On Wed, Mar 4, 2020 at 8:42 PM Vladimir Steshin <vlads...@gmail.com> wrote: > > Vyacheslav, Denis, hi again. > > > > >>> I agree with the proposal to introduce a new method which returns proxy > include the case of locally deployed services. > > > > I see one is restricted to use an interface for service with > IgniteServiceProcessor#serviceProxy(…): > > > > A.ensure(svcItf.isInterface(), "Service class must be an interface: " + > svcItf); > > > > What if we change IgniteService#serviceProxy(...) so that it will return > proxy everytime? That looks safe for user code. Doing so we might only > deprecate IgniteService#service(...). > > > > вт, 3 мар. 2020 г., 11:03 Vyacheslav Daradur <daradu...@gmail.com>: > > > Denis, finally I understood your arguments about interfaces check, thank > > you for the explanation. > > > > I agree with the proposal to introduce a new method which returns proxy > > include the case of locally deployed services. > > > > Also, such a method should be able to work in mode "local services > > preferred", perhaps with load-balancing (in case of multiple locally > > deployed instances). This allows our end-users to reach better performance. > > > > > > > > On Mon, Mar 2, 2020 at 7:51 PM Denis Mekhanikov <dmekhani...@gmail.com> > > wrote: > > > > > Vyacheslav, > > > > > > You can't make service interfaces extend > > > *org.apache.ignite.services.Service*. Currently it works perfectly if > > > *org.apache.ignite.services.Service* and a user-defined interface are > > > independent. This is actually the case in our current examples: > > > > > > > > https://github.com/apache/ignite/blob/master/examples/src/main/java/org/apache/ignite/examples/servicegrid/SimpleMapService.java > > > I mentioned the *Serializable* interface just as an example of an > > interface > > > that can be present, but it's not the one that is going to be called by a > > > user. > > > > > > What I'm trying to say is that there is no way to say whether the service > > > is going to be used through a proxy only, or usage of a local instance is > > > also possible. > > > > > > Vladimir, > > > > > > I don't like the idea, that enabling or disabling of metrics will change > > > the behaviour of the component you collect the metrics for. Such > > behaviour > > > is far from obvious. > > > > > > Nikolay, > > > > > > I agree, that such approach is valid and makes total sense. But making > > the > > > *IgniteServices#serviceProxy()* method always return a proxy instead of a > > > local instance will change the public contract. The javadoc currently > > says > > > the following: > > > > > > > If service is available locally, then local instance is returned, > > > > otherwise, a remote proxy is dynamically created and provided for the > > > > specified service. > > > > > > > > > I propose introducing a new method that will always return a service > > proxy > > > regardless of local availability, and deprecating *serviceProxy()* and > > > *service() > > > *methods. What do you think? > > > > > > Denis > > > > > > пн, 2 мар. 2020 г. в 16:08, Nikolay Izhikov <nizhi...@apache.org>: > > > > > > > Hello, Vladimir. > > > > > > > > > What if we just provide an option to disable service metrics at all? > > > > > > > > I don't think we should create an explicit property for service > > metrics. > > > > We will implement the way to disable any metrics in the scope of > > > > IGNITE-11927 [1]. > > > > > > > > > Usage of a proxy instead of service instances can lead to performance > > > > > degradation for local instances, which is another argument against > > such > > > > change. > > > > > > > > As far as I know, many and many modern frameworks use a proxy approach. > > > > Just to name one - Spring framework works with the proxy. > > > > > > > > We should measure the impact on the performance that brings > > proxy+metric > > > > and after it make the decision on local service metrics implementation. > > > > Vladimir, can you, as a contributor of this task make this measurement? > > > > > > > > [1] https://issues.apache.org/jira/browse/IGNITE-11927 > > > > > > > > пн, 2 мар. 2020 г. в 12:56, Vladimir Steshin <vlads...@gmail.com>: > > > > > > > > > Denis, Vyacheslav, hi. > > > > > > > > > > What if we just provide an option to disable service metrics at all? > > It > > > > > would keep direct references for local services. Also, we can make > > > > service > > > > > metrics disabled by default to keep current code working. A warning > > of > > > > > local service issues will be set with the option. > > > > > > > > > > пн, 2 мар. 2020 г. в 11:26, Vyacheslav Daradur <daradu...@gmail.com > > >: > > > > > > > > > > > >> Moreover, I don't see a way of implementing such a check. Are > > you > > > > > going > > > > > > to look just for any interface? What about Serializable? Will it > > do? > > > > > > > > > > > > The check should look for the interface which implements > > > > > > "org.apache.ignite.services.Service", it covers the requirement to > > be > > > > > > Serializable. > > > > > > > > > > > > >> For now though the best thing we can do is to calculate remote > > > > > > invocations only, since all of them go through a proxy. > > > > > > > > > > > > Let's introduce a system property to manage local services > > > monitoring: > > > > > > - local services monitoring will be disabled by default - to avoid > > > any > > > > > > backward compatibility issues; > > > > > > - local services monitoring can be enabled runtime with a known > > > > > limitation > > > > > > for new services for example; > > > > > > Moreover, if we introduce such a feature flag to > > > ServiceConfiguration - > > > > > > the new feature can be enabled per service separately. > > > > > > > > > > > > What do you think? > > > > > > > > > > > > > > > > > > > > > > > > On Mon, Mar 2, 2020 at 12:33 AM Denis Mekhanikov < > > > > dmekhani...@gmail.com> > > > > > > wrote: > > > > > > > > > > > >> Vladimir, Slava, > > > > > >> > > > > > >> In general, I like the idea of abstracting the service deployment > > > from > > > > > >> its usage, but there are some backward-compatibility > > considerations > > > > that > > > > > >> won't let us do so. > > > > > >> > > > > > >> Or we can declare usage of services without interfaces incorrect > > > > > >> > > > > > >> > > > > > >> I don't think we can introduce a requirement for all services to > > > have > > > > an > > > > > >> interface, unfortunately. Such change can potentially break > > existing > > > > > code, > > > > > >> since such requirement doesn't exist currently. > > > > > >> Moreover, I don't see a way of implementing such a check. Are you > > > > going > > > > > >> to look just for any interface? What about Serializable? Will it > > do? > > > > > >> > > > > > >> Usage of a proxy instead of service instances can lead to > > > performance > > > > > >> degradation for local instances, which is another argument against > > > > such > > > > > >> change. > > > > > >> > > > > > >> I think, it will make sense to make all service invocations work > > > > through > > > > > >> a proxy in Ignite 3. > > > > > >> For now though the best thing we can do is to calculate remote > > > > > >> invocations only, since all of them go through a proxy. > > > > > >> Another option is to provide a simple way for a user to account > > the > > > > > >> service invocations themselves. > > > > > >> > > > > > >> What do you guys think? > > > > > >> > > > > > >> Denis > > > > > >> > > > > > >> > > > > > >> вт, 25 февр. 2020 г. в 16:50, Vyacheslav Daradur < > > > daradu...@gmail.com > > > > >: > > > > > >> > > > > > >>> It is not a change of public API from my point of view. > > > > > >>> > > > > > >>> Also, there is a check to allow getting proxy only for an > > > interface, > > > > > not > > > > > >>> implementation. > > > > > >>> > > > > > >>> Denis, what do you think? > > > > > >>> > > > > > >>> > > > > > >>> вт, 25 февр. 2020 г. в 16:28, Vladimir Steshin < > > vlads...@gmail.com > > > >: > > > > > >>> > > > > > >>>> Vyacheslav, this is exactly what I found. I'm doing [1] (metrics > > > for > > > > > >>>> services) and realized I have to wrap local calls by a proxy. Is > > > it > > > > a > > > > > >>>> change of public API and should come with major release only? Or > > > we > > > > > can > > > > > >>>> declare usage of services without interfaces incorrect? > > > > > >>>> [1] https://issues.apache.org/jira/browse/IGNITE-12464 > > > > > >>>> > > > > > >>>> вт, 25 февр. 2020 г. в 16:17, Vyacheslav Daradur < > > > > daradu...@gmail.com > > > > > >: > > > > > >>>> > > > > > >>>>> {IgniteServices#service(String name)} returns direct reference > > in > > > > the > > > > > >>>>> current implementation. > > > > > >>>>> > > > > > >>>>> So, class casting should work for your example: > > > > > >>>>> ((MyServiceImpl)ignite.services().service(“myService”)).bar(); > > > > > >>>>> > > > > > >>>>> It is safer to use an interface instead of an implementation, > > > there > > > > > is > > > > > >>>>> no guarantee that in future releases direct link will be > > > returned, > > > > a > > > > > >>>>> service instance might be wrapped for monitoring for example. > > > > > >>>>> > > > > > >>>>> > > > > > >>>>> On Tue, Feb 25, 2020 at 4:09 PM Vladimir Steshin < > > > > vlads...@gmail.com > > > > > > > > > > > >>>>> wrote: > > > > > >>>>> > > > > > >>>>>> Vyacheslav, Hi. > > > > > >>>>>> > > > > > >>>>>> I see. But can we consider 'locally deployed service' is a > > proxy > > > > > too, > > > > > >>>>>> not direct reference? What if I need to wrap it? This would be > > > > local > > > > > >>>>>> service working via proxy or null. > > > > > >>>>>> > > > > > >>>>>> вт, 25 февр. 2020 г. в 16:03, Vyacheslav Daradur < > > > > > daradu...@gmail.com > > > > > >>>>>> >: > > > > > >>>>>> > > > > > >>>>>>> Hi, Vladimir > > > > > >>>>>>> > > > > > >>>>>>> The answer is in API docs: "Gets *locally deployed service* > > > with > > > > > >>>>>>> specified name." [1] > > > > > >>>>>>> > > > > > >>>>>>> That means {IgniteServices#service(String name)} returns only > > > > > >>>>>>> locally deployed instance or null. > > > > > >>>>>>> > > > > > >>>>>>> {IgniteServices#serviceProxy(…)} returns proxy to call > > > instances > > > > > >>>>>>> across the cluster. Might be used for load-balancing. > > > > > >>>>>>> > > > > > >>>>>>> [1] > > > > > >>>>>>> > > > > > > > > > > > > > > https://github.com/apache/ignite/blob/56975c266e7019f307bb9da42333a6db4e47365e/modules/core/src/main/java/org/apache/ignite/IgniteServices.java#L569 > > > > > >>>>>>> > > > > > >>>>>>> On Tue, Feb 25, 2020 at 3:51 PM Vladimir Steshin < > > > > > vlads...@gmail.com> > > > > > >>>>>>> wrote: > > > > > >>>>>>> > > > > > >>>>>>>> Hello, Igniters. > > > > > >>>>>>>> > > > > > >>>>>>>> Previous e-mail was with wrong topic 'daradu...@gmail.com' > > :) > > > > > >>>>>>>> > > > > > >>>>>>>> I got a question what exactly IgniteServices#service(String > > > > name) > > > > > >>>>>>>> is supposed to return: reference to the object or a proxy > > for > > > > > some reason > > > > > >>>>>>>> like IgniteServices#serviceProxy(…)? Vyacheslav D., can you > > > tell > > > > > me your > > > > > >>>>>>>> opinion? > > > > > >>>>>>>> > > > > > >>>>>>>> public interface MyService { > > > > > >>>>>>>> > > > > > >>>>>>>> public void foo(); > > > > > >>>>>>>> > > > > > >>>>>>>> } > > > > > >>>>>>>> > > > > > >>>>>>>> public class MyServiceImpl implements Service, MyService { > > > > > >>>>>>>> > > > > > >>>>>>>> @Override public void foo(){ … } > > > > > >>>>>>>> > > > > > >>>>>>>> public void bar(){ … }; > > > > > >>>>>>>> > > > > > >>>>>>>> } > > > > > >>>>>>>> > > > > > >>>>>>>> > > > > > >>>>>>>> // Is it required to support > > > > > >>>>>>>> > > > > > >>>>>>>> MyServiceImpl srvc = ignite.services().service(“myService”); > > > > > >>>>>>>> > > > > > >>>>>>>> srvc.foo(); > > > > > >>>>>>>> > > > > > >>>>>>>> srvc.bar(); > > > > > >>>>>>>> > > > > > >>>>>>>> > > > > > >>>>>>>> > > > > > >>>>>>>> // Or is the only correct way: > > > > > >>>>>>>> > > > > > >>>>>>>> MyService srvc = ignite.services().service(“myService”); > > > > > >>>>>>>> > > > > > >>>>>>>> srvc.foo(); > > > > > >>>>>>>> > > > > > >>>>>>>> > > > > > >>>>>>> > > > > > >>>>>>> -- > > > > > >>>>>>> Best Regards, Vyacheslav D. > > > > > >>>>>>> > > > > > >>>>>> > > > > > >>>>> > > > > > >>>>> -- > > > > > >>>>> Best Regards, Vyacheslav D. > > > > > >>>>> > > > > > >>>> -- > > > > > >>> Best Regards, Vyacheslav D. > > > > > >>> > > > > > >> > > > > > > > > > > > > -- > > > > > > Best Regards, Vyacheslav D. > > > > > > > > > > > > > > > > > > > > > > > > -- > > Best Regards, Vyacheslav D. > >
