Re: Proxy (GridServiceProxy) for local services if required
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 > 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 : >> > > >> > > > 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 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/`)
Re: Proxy (GridServiceProxy) for local services if required
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 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 : > > > > > > > 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 >: > > > > > > > > > > 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 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
Re: Proxy (GridServiceProxy) for local services if required
Yep, seems that current behaviour is not correct at all. `serviceProxy()` should return exactly what it should -- proxy. ср, 19 янв. 2022 г. в 10:32, Maksim Timonin : > 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 : > > > > > > > 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 >: > > > > > > > > > > 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 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
