Hi Maksim, Which patch are you referring to?
-Val On Mon, Jan 24, 2022 at 10:36 AM Maksim Timonin <timoninma...@apache.org> wrote: > Hi guys, > > > If we already agreed to deprecate the service() method, then I'm ok with > the change > > As I can see in the previous discussion that Vladimir mentioned [1], there > was a proposal for deprecating by Vladimir and Denis Mekhanikov, but there > wasn't a final decision. So I tried to see the `#service()` from a > different angle and tried to be a user's advocate. But I found only a > few weak arguments for remaining this functionality, and also nobody > supported them :) So, if there are no objections let's finally deprecate > it. > > > But I think we should do both fixes together and also clearly document > that using service() can lead to incorrect metrics > > Did I correctly understand that: we need to revert the previous patch, > update it with the deprecation, and submit it again to master? > > > > On Sun, Jan 23, 2022 at 3:51 AM Valentin Kulichenko < > valentin.kuliche...@gmail.com> wrote: > > > If we already agreed to deprecate the service() method, then I'm ok with > > the change. But I think we should do both fixes together and also clearly > > document that using service() can lead to incorrect metrics. > > > > -Val > > > > On Fri, Jan 21, 2022 at 1:13 AM Vladimir Steshin <vlads...@gmail.com> > > wrote: > > > > > Valentin, there are 2 notable issues: > > > > > > 1) Variate behavior of `/serviceProxy()/` depending on user setting. It > > > can return both proxy and reference. > > > > > > 2) Service metrics are corrupted by invocations through `/service()/`. > > > > > > > > > How we can fix: > > > > > > 1) Just return proxy every time. > > > > > > 2) Deprecate `/service()/`. We've already discussed that here: [1]. > > > Please see my previous message in the thread. > > > > > > > > > > > > [1] https://www.mail-archive.com/dev@ignite.apache.org/msg44062.html > > > > > > > > > On 20.01.2022 22:48, Valentin Kulichenko 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 > > > >>>>>>>>>> > > > >>>>>>>>>> > > >
