Vladimir, I think we should make the same changes to the .NET API. On Tue, Jan 25, 2022 at 5:17 PM Vladimir Steshin <vlads...@gmail.com> wrote:
> Similar question about .NET. I'm doing the sub-ticket with metrics > of the platform services. Should we return proxy every tome by > /IServices.GerServiceProxy()/ and probably deprecate > /IServices.GetService()/. Same problems with variate the behavior of > /GerServiceProxy()/ and statistics corruption (if enabled) with > /GetService()/. > > 24.01.2022 22:27, Valentin Kulichenko пишет: > > 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 > >>>>>>>>>>>>>> > >>>>>>>>>>>>>>
