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 > > >>>>>>>>>> > > >>>>>>>>>> >
