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