>>>Did I correctly understand that: we need to revert the previous
patch, update it with the deprecation, and submit it again to master?
Not sure at all. For what? The patch could be improved. But it is
independent, brings new features with documented limitations. If we want
to deprecate something, let's do it. No need, imho, to revert anything.
Next version like 2.13 would contain all there fixes combined.
24.01.2022 21:35, Maksim Timonin пишет:
Hi guys,
If we already agreed to deprecate the service() method, then I'm ok with
the change
As I can see in the previous discussion that Vladimir mentioned [1], there
was a proposal for deprecating by Vladimir and Denis Mekhanikov, but there
wasn't a final decision. So I tried to see the `#service()` from a
different angle and tried to be a user's advocate. But I found only a
few weak arguments for remaining this functionality, and also nobody
supported them :) So, if there are no objections let's finally deprecate it.
But I think we should do both fixes together and also clearly document
that using service() can lead to incorrect metrics
Did I correctly understand that: we need to revert the previous patch,
update it with the deprecation, and submit it again to master?
On Sun, Jan 23, 2022 at 3:51 AM Valentin Kulichenko <
valentin.kuliche...@gmail.com> wrote:
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
