Andrey, hi. >> We won’t affect existing services
How exactly will we affect services without special interface? Please see the benchmarks in previous email. >>> what if we generate will generate proxy that collects service’s metrics only if service will implement some special interface? I don’t like idea enabling/disabling metrics involves code change, compilation. I believe it should be an external option, probably available at runtime through JMX. >> we can impose additional requirement for services that want use metrics out of box. … service must have own interface and only invocations of methods of this interface will be taken into account for metrics collection. Why one more interface? To work via proxy, with remote services user already has to use an interface additionally to Service. If we introduce proxy for local services too (as suggested earlier), an interface will be required. Current IgniteService#serviceProxy() already requires interface even for local service. I don’t think we need one more special interface. >> user always can use own metrics framework. Since we do not significantly affect services user can use both or disable our by an option. With the discussion before and the benchmark I propose: - Let IgniteService#serviceProxy() give GridServiceProxy for local services too. It already requires to work via interface. So it’s safe for user code. - Deprecate IgniteService#service() - Make service metrics enabled by default for all services. - Bring system param which disables metrics by default for all services. - Bring parameter/method in MetricsMxBean which allows disabling/enabling metrics for all services at run time. Makes sense? чт, 5 мар. 2020 г., 16:48 Andrey Gura <ag...@apache.org>: > 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. > > > >
