Vladimir, Agreed. My point is that the actual issue we want to fix is broken metrics. Thus, we should have a single ticket/patch *about the broken metrics* that incorporates all required changes, as well as relevant documentation updates.
I don't think we need a vote. I will also change mine to +1 in the existing thread. -Val On Mon, Jan 24, 2022 at 11:23 AM Vladimir Steshin <vlads...@gmail.com> wrote: > Sure. This is a simple change. These 2 fixes could go together. And > the doc already has notes of service statistics traits. Ok, might be > even better. > > I'm not sure whether we need extra voting for deprecating > 'service()' and, as earlier and making 'serviceProxy()' return proxy > every time. Several active contributors have said their 'yes' for both > in the thread. > > Several active contributors have already said their 'yes'. > > 23.01.2022 03:50, Valentin Kulichenko пишет: > > 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 > >>>>>>>>>>>> > >>>>>>>>>>>> >
