Hi guys, > because it's still possible to get a local instance via the service() method
I think that there are some cases why a user doesn't want to have a proxy: 1. the user actually cares about proxy performance. (Vladimir measured it here [1] and found that proxy actually affects some simple cases of services usage. But do we actually need measure such cases?) 2. the user wants to work with implementation classes over interfaces for any reason. (Bad practice in any case, I think). >From my point of view those cases are very rare. May I miss some cases here? There was a commit [2] that provides this optimization in `serviceProxy()`. But I don't see any motivation in the commit message, also the ticket is GG private. Which case is it covered? If there are no obvious cases then it looks like we can safely deprecate the `service()` method. But if there are some, then we can remain the `service()` method for backward compatibility for users who actually understand what they are doing. With mention that it's impossible to measure it. [1] https://www.mail-archive.com/dev@ignite.apache.org/msg43793.html [2] https://github.com/apache/ignite/commit/ef6b8aa3 On Thu, Jan 20, 2022 at 10:49 PM Valentin Kulichenko < valentin.kuliche...@gmail.com> wrote: > So the proposed change will not actually fix the issue with metrics, > because it's still possible to get a local instance via the service() > method. At the same time, the change removes an existing performance > optimization. > > Let's figure out how to fix the actual problem. If the *only* way to have > metrics is to have a proxy, then this should be the only way to interact > with a service. In that case, we need to do something with the service() > method (deprecate it?). Or probably there are other ways to fix metrics? > > -Val > > On Thu, Jan 20, 2022 at 3:32 AM Vladimir Steshin <vlads...@gmail.com> > wrote: > > > Yes. Invocations from direct reference are not measured. This method > > in the javadoc: > > > > > > /* <b>NOTE:</b> Statistics are collected only with service proxies > > obtaining by methods like/ > > > > /* {@link IgniteServices#serviceProxy(String, Class, boolean)} and won't > > work for direct reference of local/ > > > > /* services which you can get by, for example, {@link > > IgniteServices#service(String)}./ > > > > > > On 20.01.2022 00:20, Valentin Kulichenko wrote: > > > BTW, there is also the service() method that can only return an > instance > > > and never returns a proxy. Does it corrupt the metrics as well? > > > > > > -Val > > > > > > On Wed, Jan 19, 2022 at 1:09 PM Valentin Kulichenko < > > > valentin.kuliche...@gmail.com> wrote: > > > > > >> Maxim, > > >> > > >> The reason I'm asking is that I don't really understand how client > side > > >> mechanics affect server side metrics (number of executions and their > > >> durations). I feel that we might be fixing a wrong problem. > > >> > > >> Could you elaborate on why we count metrics incorrectly when > > >> the serviceProxy() returns an instance of a local service instead of > an > > >> actual proxy? > > >> > > >> -Val > > >> > > >> On Tue, Jan 18, 2022 at 11:32 PM Maksim Timonin< > timoninma...@apache.org > > > > > >> wrote: > > >> > > >>> Hi, guys! > > >>> > > >>>> this is not a good idea to change the behavior of serviceProxy() > > >>> depending on statistics > > >>> > > >>> I think that the patch doesn't change the behavior of > `serviceProxy()`. > > >>> This method promises a proxy and it actually returns it. The fact > that > > >>> `serviceProxy()` can return non-proxy objects is an internal Ignite > > >>> optimization, and users should not rely on this, there is a separate > > >>> method > > >>> `service()` for that. > > >>> > > >>>> What are the metrics that are being affected by this? > > >>> Only service metrics, that calculates duration of service execution. > > Check > > >>> this ticket [1] > > >>> > > >>> [1]https://issues.apache.org/jira/browse/IGNITE-12464 > > >>> > > >>> > > >>> On Wed, Jan 19, 2022 at 1:22 AM Valentin Kulichenko < > > >>> valentin.kuliche...@gmail.com> wrote: > > >>> > > >>>> What are the metrics that are being affected by this? > > >>>> > > >>>> -Val > > >>>> > > >>>> On Tue, Jan 18, 2022 at 3:31 AM Вячеслав Коптилин < > > >>>> slava.kopti...@gmail.com> > > >>>> wrote: > > >>>> > > >>>>> Hello Igniters, > > >>>>> > > >>>>> IMHO, this is not a good idea to change the behavior of > > serviceProxy() > > >>>>> depending on statistics (enabled/disabled). It seems > counterintuitive > > >>> to > > >>>>> me. > > >>>>> Perhaps, we need to introduce a new method that should always > return > > a > > >>>>> proxy to the user service. > > >>>>> > > >>>>> Thanks, > > >>>>> Slava. > > >>>>> > > >>>>> > > >>>>> вт, 28 дек. 2021 г. в 13:57, Pavel Pereslegin<xxt...@gmail.com>: > > >>>>> > > >>>>>> Hi! > > >>>>>> > > >>>>>> Agree with Maxim. > > >>>>>> > > >>>>>> It seems to me quite normal to return a proxy for a local instance > > >>> in > > >>>>>> the case when the user has explicitly enabled statistics > collection > > >>> in > > >>>>>> the service settings. Those. by default, we do not change the > > >>> behavior > > >>>>>> and if the collection of metrics is not needed, a local instance > > >>> will > > >>>>>> be returned. And I also think the javadoc should be changed to > > >>> reflect > > >>>>>> the new behavior. > > >>>>>> > > >>>>>> So, I'm for 1 + 3. > > >>>>>> > > >>>>>> вт, 28 дек. 2021 г. в 10:51, Maksim Timonin < > > >>> timoninma...@apache.org>: > > >>>>>>> Hi! > > >>>>>>> > > >>>>>>> I agree that users shouldn't expect a non-proxy when invoking the > > >>>>>>> `IgniteServices#serviceProxy()` method. I think it's up to Ignite > > >>> to > > >>>>>> return > > >>>>>>> a non-proxy instance here as possible optimisation. But users > > >>> have to > > >>>>> use > > >>>>>>> interfaces in any case. There is the `IgniteServices#service()` > > >>>> method > > >>>>>> for > > >>>>>>> explicit return of local instances. > > >>>>>>> > > >>>>>>> With enabling of metrics we can break users that explicitly > > >>>>>>> use `#serviceProxy` (proxy!), and then explicitly cast it to an > > >>>>>>> implementation class. In this case such users will get a runtime > > >>>>>> exception. > > >>>>>>> I think we can write a good javadoc for > > >>>>>>> `ServiceConfiguration#setEnableMetrics()`, it should mention that > > >>> it > > >>>>>> works > > >>>>>>> only with proxy, and it doesn't collect metrics with non-proxy > > >>> usages > > >>>>>> with > > >>>>>>> `IgniteService#service()`. > > >>>>>>> > > >>>>>>> So, I propose to proceed with two solutions - 1 and 3: fix docs > > >>> for > > >>>>>>> `#serviceProxy()` and provide detailed javadocs > > >>>>>>> for `ServiceConfiguration#setEnableMetrics()`. > > >>>>>>> > > >>>>>>> If some users will enable metrics (even with such docs!) and will > > >>> be > > >>>>>> using > > >>>>>>> casting proxy(!) to an implementation, then they will get a > > >>> runtime > > >>>>>>> exception. But I believe that it is an obvious failure, and it > > >>> should > > >>>>> be > > >>>>>>> fixed on the user side. > > >>>>>>> > > >>>>>>> > > >>>>>>> > > >>>>>>> > > >>>>>>> > > >>>>>>> On Mon, Dec 27, 2021 at 10:26 PM Vladimir Steshin < > > >>>> vlads...@gmail.com> > > >>>>>>> wrote: > > >>>>>>> > > >>>>>>>> Hi, Igniters. > > >>>>>>>> > > >>>>>>>> > > >>>>>>>> I'd like to suggest modifying > > >>> `/IgniteServices.serviceProxy(String > > >>>>>> name, > > >>>>>>>> Class<? super T> svcItf, boolean sticky)/` so that it may return > > >>>>> proxy > > >>>>>>>> even for local service. Motivation: service metrics [1]. To > > >>> measure > > >>>>>>>> method call we need to wrap service somehow. Also, the method > > >>> name > > >>>>> says > > >>>>>>>> `proxy`. For local one we return direct instance. Misleading. > > >>>>>>>> > > >>>>>>>> > > >>>>>>>> Solutions: > > >>>>>>>> > > >>>>>>>> 1) Let’s return a proxy (`/GridServiceProxy/`) once we need. > > >>> Let’s > > >>>>>>>> change the javadoc to say `@return Proxy over service’. Simply > > >>>> works. > > >>>>>>>> Pros: invocation like `/MyServiceImpl svc = > > >>>> serviceProxy(«myservice», > > >>>>>>>> MyService.class)`/ would fail. Wierd usage to me. But possible. > > >>>>>>>> > > >>>>>>>> 2) Introduce a new method with forced-proxy flag > > >>>>>>>> like`IgniteSerives.serviceProxy(…, boolead focedProxy)`. And > > >>> add a > > >>>>>>>> warning to other service-obtain methods like: «`/You enabled > > >>>> service > > >>>>>>>> metrics but it doesn’t work for local service instanse. Use > > >>>>>> forced-proxy > > >>>>>>>> serviceProxy(…, boolean forcedProxy)/` Pros: we’ve already have > > >>>> about > > >>>>>>>> 5-6 methods to get services in `/IgniteServices/`. I doubt we > > >>> need > > >>>>> one > > >>>>>>>> more. > > >>>>>>>> > > >>>>>>>> 3) Fix the documentation so that it tells the service metrics > > >>> would > > >>>>>> work > > >>>>>>>> only with proxy. Pros: service metrics just won’t work for local > > >>>>>>>> services. Suddenly. > > >>>>>>>> > > >>>>>>>> > > >>>>>>>> My vote is for #1: let's use a proxy. WDYT? > > >>>>>>>> > > >>>>>>>> > > >>>>>>>> [1]https://issues.apache.org/jira/browse/IGNITE-12464 > > >>>>>>>> > > >>>>>>>> >
