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