Re: Internal classes are exposed in public API
The discussion thread is started: http://apache-ignite-developers.2346864.n4.nabble.com/DISCUSS-Public-API-deprecation-rules-td45647.html ср, 5 февр. 2020 г. в 12:43, Alexey Goncharuk : > Sorry for the rush, I think I got it - it's right in the voting process > [1]. This would be a vote on a procedural issue of how to handle code > deprecation. > > I'll start the discussion soon, let's agree on the alternatives and then > start the vote. > > [1] https://www.apache.org/foundation/voting.html >
Re: Internal classes are exposed in public API
Sorry for the rush, I think I got it - it's right in the voting process [1]. This would be a vote on a procedural issue of how to handle code deprecation. I'll start the discussion soon, let's agree on the alternatives and then start the vote. [1] https://www.apache.org/foundation/voting.html
Re: Internal classes are exposed in public API
Denis, I just realized that this vote would not be a regular Apache vote we used to run. Usually we vote for a change to be accepted or rejected. Here, on the other hand, we need to choose between two alternatives, and I am not sure yet how this should be formally handled. Dmitriy, perhaps, you can shed some light on this? Meanwhile, I'll brush up on the voting process as well.
Re: Internal classes are exposed in public API
Let's run a vote if that's the only option to come to a consensus. It will be best if either Alex Goncharuk, Andrey Gura or Nikolay Izhikov creates a discussion thread stating the problem and possible choices. Folks, who would like to take over? Generally, the vote should help us to decide how to deal with similar situations in the future. We need to agree on how and in what circumstances we deprecate existing APIs and engrave this on our wiki. As for this discussion related to the new metrics framework, it should be used just as a reference. - Denis On Tue, Feb 4, 2020 at 2:38 AM Maxim Muzafarov wrote: > Folks, > > Let's start a vote? > > On Mon, 3 Feb 2020 at 15:05, Andrey Gura wrote: > > > > Just post here article from Oracle documentation "How and When To > > Deprecate APIs" [1]. > > > > [1] > https://docs.oracle.com/javase/8/docs/technotes/guides/javadoc/deprecation/deprecation.html > > > > On Fri, Jan 31, 2020 at 10:44 AM Pavel Tupitsyn > wrote: > > > > > > Agree with Andrey, let's remove deprecation for now and unblock the > release. > > > > > > On Thu, Jan 30, 2020 at 10:23 PM Andrey Gura wrote: > > > > > > > I'll just repeat one thought with some changes. > > > > > > > > There are at least two groups of people in this discussion. One group > > > > sure that new API is complete and production ready while other group > > > > disagree with it. Obviously we can't reach consensus about it right > > > > now. But we can do it in the future. > > > > At present we just can't deprecate existing API and mark new API as > > > > experimental at the same time. So we must remove deprecation until > the > > > > consensus is reached. > > > > > > > > Also there is the third group of people. This people aren't related > > > > with the API, they may be don't need the API and they wait for bug > > > > fixes and other features. It is very easy to satisfy third group: > just > > > > cut off what caused the release blocking. But it is much easier to > > > > remove @deprecated annotations. > > > > > > > > On Thu, Jan 30, 2020 at 8:54 PM Nikolay Izhikov > > > > > wrote: > > > > > > > > > > Alexey. > > > > > > > > > > I answered to your examples and issues you provide. > > > > > But, it seems the discussion of the API and the Java code itself > is not > > > > the goal of this thread anymore. > > > > > > > > > > > Should we provide a way to know the number of metrics and > registries > > > > in advance? > > > > > > > > > > No. > > > > > If you think this is the real use-as let’s add `size` methods. > > > > > It will be the simple API *extension*. > > > > > > > > > > > There is no separation on public and internal metrics > > > > > > > > > > Any metric can be changed(removed) in any time. > > > > > But we will try not to do it unless we have a strong reason. > > > > > > > > > > > Will we allow users to register their own metrics? > > > > > > > > > > No. > > > > > > > > > > > It's still not clear how a user will map old interfaces and > methods to > > > > the new metric names. > > > > > > > > > > We should write this information in the deprecation message. > > > > > > > > > > > 30 янв. 2020 г., в 20:27, Alexey Goncharuk < > alexey.goncha...@gmail.com> > > > > написал(а): > > > > > > > > > > > > Nikita, > > > > > > > > > > > > Disagree here. I already gave an example in this thread of how > you > > > > need to > > > > > > peek into configuration in order to obtain an instance of > exporter SPI > > > > > > which may not necessarily be the type you need. That's why > > > > IGNITE-12553 was > > > > > > created in the first place. > > > > > > > > > >
Re: Internal classes are exposed in public API
Folks, Let's start a vote? On Mon, 3 Feb 2020 at 15:05, Andrey Gura wrote: > > Just post here article from Oracle documentation "How and When To > Deprecate APIs" [1]. > > [1] > https://docs.oracle.com/javase/8/docs/technotes/guides/javadoc/deprecation/deprecation.html > > On Fri, Jan 31, 2020 at 10:44 AM Pavel Tupitsyn wrote: > > > > Agree with Andrey, let's remove deprecation for now and unblock the release. > > > > On Thu, Jan 30, 2020 at 10:23 PM Andrey Gura wrote: > > > > > I'll just repeat one thought with some changes. > > > > > > There are at least two groups of people in this discussion. One group > > > sure that new API is complete and production ready while other group > > > disagree with it. Obviously we can't reach consensus about it right > > > now. But we can do it in the future. > > > At present we just can't deprecate existing API and mark new API as > > > experimental at the same time. So we must remove deprecation until the > > > consensus is reached. > > > > > > Also there is the third group of people. This people aren't related > > > with the API, they may be don't need the API and they wait for bug > > > fixes and other features. It is very easy to satisfy third group: just > > > cut off what caused the release blocking. But it is much easier to > > > remove @deprecated annotations. > > > > > > On Thu, Jan 30, 2020 at 8:54 PM Nikolay Izhikov > > > wrote: > > > > > > > > Alexey. > > > > > > > > I answered to your examples and issues you provide. > > > > But, it seems the discussion of the API and the Java code itself is not > > > the goal of this thread anymore. > > > > > > > > > Should we provide a way to know the number of metrics and registries > > > in advance? > > > > > > > > No. > > > > If you think this is the real use-as let’s add `size` methods. > > > > It will be the simple API *extension*. > > > > > > > > > There is no separation on public and internal metrics > > > > > > > > Any metric can be changed(removed) in any time. > > > > But we will try not to do it unless we have a strong reason. > > > > > > > > > Will we allow users to register their own metrics? > > > > > > > > No. > > > > > > > > > It's still not clear how a user will map old interfaces and methods to > > > the new metric names. > > > > > > > > We should write this information in the deprecation message. > > > > > > > > > 30 янв. 2020 г., в 20:27, Alexey Goncharuk > > > > > > > > написал(а): > > > > > > > > > > Nikita, > > > > > > > > > > Disagree here. I already gave an example in this thread of how you > > > need to > > > > > peek into configuration in order to obtain an instance of exporter SPI > > > > > which may not necessarily be the type you need. That's why > > > IGNITE-12553 was > > > > > created in the first place. > > > > > > >
Re: Internal classes are exposed in public API
Just post here article from Oracle documentation "How and When To Deprecate APIs" [1]. [1] https://docs.oracle.com/javase/8/docs/technotes/guides/javadoc/deprecation/deprecation.html On Fri, Jan 31, 2020 at 10:44 AM Pavel Tupitsyn wrote: > > Agree with Andrey, let's remove deprecation for now and unblock the release. > > On Thu, Jan 30, 2020 at 10:23 PM Andrey Gura wrote: > > > I'll just repeat one thought with some changes. > > > > There are at least two groups of people in this discussion. One group > > sure that new API is complete and production ready while other group > > disagree with it. Obviously we can't reach consensus about it right > > now. But we can do it in the future. > > At present we just can't deprecate existing API and mark new API as > > experimental at the same time. So we must remove deprecation until the > > consensus is reached. > > > > Also there is the third group of people. This people aren't related > > with the API, they may be don't need the API and they wait for bug > > fixes and other features. It is very easy to satisfy third group: just > > cut off what caused the release blocking. But it is much easier to > > remove @deprecated annotations. > > > > On Thu, Jan 30, 2020 at 8:54 PM Nikolay Izhikov > > wrote: > > > > > > Alexey. > > > > > > I answered to your examples and issues you provide. > > > But, it seems the discussion of the API and the Java code itself is not > > the goal of this thread anymore. > > > > > > > Should we provide a way to know the number of metrics and registries > > in advance? > > > > > > No. > > > If you think this is the real use-as let’s add `size` methods. > > > It will be the simple API *extension*. > > > > > > > There is no separation on public and internal metrics > > > > > > Any metric can be changed(removed) in any time. > > > But we will try not to do it unless we have a strong reason. > > > > > > > Will we allow users to register their own metrics? > > > > > > No. > > > > > > > It's still not clear how a user will map old interfaces and methods to > > the new metric names. > > > > > > We should write this information in the deprecation message. > > > > > > > 30 янв. 2020 г., в 20:27, Alexey Goncharuk > > написал(а): > > > > > > > > Nikita, > > > > > > > > Disagree here. I already gave an example in this thread of how you > > need to > > > > peek into configuration in order to obtain an instance of exporter SPI > > > > which may not necessarily be the type you need. That's why > > IGNITE-12553 was > > > > created in the first place. > > > > >
Re: Internal classes are exposed in public API
Agree with Andrey, let's remove deprecation for now and unblock the release. On Thu, Jan 30, 2020 at 10:23 PM Andrey Gura wrote: > I'll just repeat one thought with some changes. > > There are at least two groups of people in this discussion. One group > sure that new API is complete and production ready while other group > disagree with it. Obviously we can't reach consensus about it right > now. But we can do it in the future. > At present we just can't deprecate existing API and mark new API as > experimental at the same time. So we must remove deprecation until the > consensus is reached. > > Also there is the third group of people. This people aren't related > with the API, they may be don't need the API and they wait for bug > fixes and other features. It is very easy to satisfy third group: just > cut off what caused the release blocking. But it is much easier to > remove @deprecated annotations. > > On Thu, Jan 30, 2020 at 8:54 PM Nikolay Izhikov > wrote: > > > > Alexey. > > > > I answered to your examples and issues you provide. > > But, it seems the discussion of the API and the Java code itself is not > the goal of this thread anymore. > > > > > Should we provide a way to know the number of metrics and registries > in advance? > > > > No. > > If you think this is the real use-as let’s add `size` methods. > > It will be the simple API *extension*. > > > > > There is no separation on public and internal metrics > > > > Any metric can be changed(removed) in any time. > > But we will try not to do it unless we have a strong reason. > > > > > Will we allow users to register their own metrics? > > > > No. > > > > > It's still not clear how a user will map old interfaces and methods to > the new metric names. > > > > We should write this information in the deprecation message. > > > > > 30 янв. 2020 г., в 20:27, Alexey Goncharuk > написал(а): > > > > > > Nikita, > > > > > > Disagree here. I already gave an example in this thread of how you > need to > > > peek into configuration in order to obtain an instance of exporter SPI > > > which may not necessarily be the type you need. That's why > IGNITE-12553 was > > > created in the first place. > > >
Re: Internal classes are exposed in public API
I'll just repeat one thought with some changes. There are at least two groups of people in this discussion. One group sure that new API is complete and production ready while other group disagree with it. Obviously we can't reach consensus about it right now. But we can do it in the future. At present we just can't deprecate existing API and mark new API as experimental at the same time. So we must remove deprecation until the consensus is reached. Also there is the third group of people. This people aren't related with the API, they may be don't need the API and they wait for bug fixes and other features. It is very easy to satisfy third group: just cut off what caused the release blocking. But it is much easier to remove @deprecated annotations. On Thu, Jan 30, 2020 at 8:54 PM Nikolay Izhikov wrote: > > Alexey. > > I answered to your examples and issues you provide. > But, it seems the discussion of the API and the Java code itself is not the > goal of this thread anymore. > > > Should we provide a way to know the number of metrics and registries in > > advance? > > No. > If you think this is the real use-as let’s add `size` methods. > It will be the simple API *extension*. > > > There is no separation on public and internal metrics > > Any metric can be changed(removed) in any time. > But we will try not to do it unless we have a strong reason. > > > Will we allow users to register their own metrics? > > No. > > > It's still not clear how a user will map old interfaces and methods to the > > new metric names. > > We should write this information in the deprecation message. > > > 30 янв. 2020 г., в 20:27, Alexey Goncharuk > > написал(а): > > > > Nikita, > > > > Disagree here. I already gave an example in this thread of how you need to > > peek into configuration in order to obtain an instance of exporter SPI > > which may not necessarily be the type you need. That's why IGNITE-12553 was > > created in the first place. >
Re: Internal classes are exposed in public API
Alexey. I answered to your examples and issues you provide. But, it seems the discussion of the API and the Java code itself is not the goal of this thread anymore. > Should we provide a way to know the number of metrics and registries in > advance? No. If you think this is the real use-as let’s add `size` methods. It will be the simple API *extension*. > There is no separation on public and internal metrics Any metric can be changed(removed) in any time. But we will try not to do it unless we have a strong reason. > Will we allow users to register their own metrics? No. > It's still not clear how a user will map old interfaces and methods to the > new metric names. We should write this information in the deprecation message. > 30 янв. 2020 г., в 20:27, Alexey Goncharuk > написал(а): > > Nikita, > > Disagree here. I already gave an example in this thread of how you need to > peek into configuration in order to obtain an instance of exporter SPI > which may not necessarily be the type you need. That's why IGNITE-12553 was > created in the first place.
Re: Internal classes are exposed in public API
Folks, seriously, we should deprecate an existing API only after the new one is no longer considered experimental. There might be API and configuration changes in the experimental API before it's announced as GA. I would encourage us to do this properly - let's release the new APIs labeling them as experimental for now and not label the existing as deprecated until the new is announced as GA. - Denis On Thu, Jan 30, 2020 at 9:27 AM Alexey Goncharuk wrote: > Nikita, > > Disagree here. I already gave an example in this thread of how you need to > peek into configuration in order to obtain an instance of exporter SPI > which may not necessarily be the type you need. That's why IGNITE-12553 was > created in the first place. >
Re: Internal classes are exposed in public API
Nikita, Disagree here. I already gave an example in this thread of how you need to peek into configuration in order to obtain an instance of exporter SPI which may not necessarily be the type you need. That's why IGNITE-12553 was created in the first place.
Re: Internal classes are exposed in public API
Folks,
I have some experience using the new metrics API. The new format is
easy to use from the user point and approved by the community. It
provides a single metric format and one entry point. I believe that
they are able to replace existing ones. I don't see any issues with
the new metrics. The MetricExporterSpi provides the ability to easily
migrate to the new format.
So, I suggest:
1. Do NOT mark the new metric API as IgniteExperimental.
2. Mark existing metrics with Deprecated annotation with reference on
how to use and migrate to the new metrics (without reference to
internal classes).
чт, 30 янв. 2020 г. в 16:02, Andrey Gura :
>
> > We doesn’t come to an agreement that API should be changed.
> > We should discuss the design of the Metric API and your proposals for it in
> > another thread.
> > Please, avoid arguments like «this API will be 100% changed» in this
> > discussion.
>
> I just explain why this item is acceptable to me. I don't discuss how
> exactly something will changed, etc.
>
> On Thu, Jan 30, 2020 at 2:34 PM Nikolay Izhikov wrote:
> >
> > Hello.
> >
> > > 2. Add @IgniteExperimetnal to new API (because... see item. 1
> >
> > +1
> >
> > > 1. Remove @Deprecated from old API (because it strange to have one
> > > deprecated API and second experimental API)
> >
> > -1
> >
> > I propose to update deprecation message and provide metric name for each
> > deprecated method.
> >
> > > @deprecated Use {@link JmxMetricExporterSPI} instead. Name of the metric
> > > «io.dataregion.pageCount»
> >
> > Andrey.
> >
> > We doesn’t come to an agreement that API should be changed.
> > We should discuss the design of the Metric API and your proposals for it in
> > another thread.
> >
> > Please, avoid arguments like «this API will be 100% changed» in this
> > discussion.
> >
> > > 30 янв. 2020 г., в 14:21, Andrey Gura написал(а):
> > >
> > > From my point of view we still don't have consensus.
> > >
> > > I think we should do at least the following:
> > >
> > > 1. Remove @Deprecated from old API (because it strange to have one
> > > deprecated API and second experimental API)
> > > 2. Add @IgniteExperimetnal to new API (because... see item. 1)
> > > 3. Do not merge IGNITE-12553 (because it adds new public interface
> > > that 100% will be changed)
> > >
> > > ideally we should also:
> > >
> > > 4. Add metrics that available only via new API to the old API (because
> > > otherwise we force user interact with both API's)
> > >
> > > On Thu, Jan 30, 2020 at 12:35 PM Alexey Goncharuk
> > > wrote:
> > >>
> > >> Folks,
> > >>
> > >> I tried to re-read the whole thread and honestly got lost at the end :)
> > >> Do
> > >> we have a consensus (if yes, what are the steps?) or should we have a
> > >> call
> > >> as Maxim suggested?
> > >>
> > >> I think it is in our best interest to get this agreed upon fast to
> > >> release
> > >> AI 2.8 soon.
> >
--
Best wishes,
Amelchev Nikita
Re: Internal classes are exposed in public API
> We doesn’t come to an agreement that API should be changed.
> We should discuss the design of the Metric API and your proposals for it in
> another thread.
> Please, avoid arguments like «this API will be 100% changed» in this
> discussion.
I just explain why this item is acceptable to me. I don't discuss how
exactly something will changed, etc.
On Thu, Jan 30, 2020 at 2:34 PM Nikolay Izhikov wrote:
>
> Hello.
>
> > 2. Add @IgniteExperimetnal to new API (because... see item. 1
>
> +1
>
> > 1. Remove @Deprecated from old API (because it strange to have one
> > deprecated API and second experimental API)
>
> -1
>
> I propose to update deprecation message and provide metric name for each
> deprecated method.
>
> > @deprecated Use {@link JmxMetricExporterSPI} instead. Name of the metric
> > «io.dataregion.pageCount»
>
> Andrey.
>
> We doesn’t come to an agreement that API should be changed.
> We should discuss the design of the Metric API and your proposals for it in
> another thread.
>
> Please, avoid arguments like «this API will be 100% changed» in this
> discussion.
>
> > 30 янв. 2020 г., в 14:21, Andrey Gura написал(а):
> >
> > From my point of view we still don't have consensus.
> >
> > I think we should do at least the following:
> >
> > 1. Remove @Deprecated from old API (because it strange to have one
> > deprecated API and second experimental API)
> > 2. Add @IgniteExperimetnal to new API (because... see item. 1)
> > 3. Do not merge IGNITE-12553 (because it adds new public interface
> > that 100% will be changed)
> >
> > ideally we should also:
> >
> > 4. Add metrics that available only via new API to the old API (because
> > otherwise we force user interact with both API's)
> >
> > On Thu, Jan 30, 2020 at 12:35 PM Alexey Goncharuk
> > wrote:
> >>
> >> Folks,
> >>
> >> I tried to re-read the whole thread and honestly got lost at the end :) Do
> >> we have a consensus (if yes, what are the steps?) or should we have a call
> >> as Maxim suggested?
> >>
> >> I think it is in our best interest to get this agreed upon fast to release
> >> AI 2.8 soon.
>
Re: Internal classes are exposed in public API
Hi Igniters,
Let me share some thoughts. Abstractly.
1. @Experimental API. When user see API marked as experimental usually it
is a red flag for using such API in prod
and also he can't count the API stability:
- API itself if not stable and one can have compilation issues with next
versions at least
- API semantic can be changed that can cause full API usage revision and
even rewrite code that use this API.
So, user expects API (and its implementation) is offered to user e.g. to
check if some user issues can be resolved with new API
or e.g. for review to developers can get some feedback.
2. @Deprecated API. From my point of view: Deprecation is not about "we
mark code to inform user\dev about legacy".
When user see deprecation he interpret this as a strict directive to use a
new API.
He expects the API is still workable (unless there is no notes that API is
totaly broken of course), but definitely expects a new stable API must
exist.
It is ok if old API have less capabilities or even have huge limitations.
Concrete.
1. Let's mark new metrics API as Experimental. This allow us to add any
changes in future minor releases to resolve known issues.
2. AFAIR, we have strong commitment to not change\delete old API between
minor releases.
We can't mark old metrics as deprecated regarding user expectations
described before.
Let's make old API as deprecated in *one of future minor* release, once a
new one will be considered as *stable*.
I'm not sure we even can forbid anyone to contribute to old API for now and
then until (in 3.0) it will be decided old API for removal via Voting.
On Thu, Jan 30, 2020 at 2:47 PM Maxim Muzafarov wrote:
> Igniters,
>
>
> How about to start a vote (formal) over the whole community to choose
> between these two options we have? With one caveat that veto votes are not
> applicable to this discussion. Let the whole community decide what
> direction we should move on.
>
> On Thu, 30 Jan 2020 at 14:34, Nikolay Izhikov wrote:
>
> > Hello.
> >
> > > 2. Add @IgniteExperimetnal to new API (because... see item. 1
> >
> > +1
> >
> > > 1. Remove @Deprecated from old API (because it strange to have one
> > deprecated API and second experimental API)
> >
> > -1
> >
> > I propose to update deprecation message and provide metric name for each
> > deprecated method.
> >
> > > @deprecated Use {@link JmxMetricExporterSPI} instead. Name of the
> metric
> > «io.dataregion.pageCount»
> >
> > Andrey.
> >
> > We doesn’t come to an agreement that API should be changed.
> > We should discuss the design of the Metric API and your proposals for it
> > in another thread.
> >
> > Please, avoid arguments like «this API will be 100% changed» in this
> > discussion.
> >
> > > 30 янв. 2020 г., в 14:21, Andrey Gura написал(а):
> > >
> > > From my point of view we still don't have consensus.
> > >
> > > I think we should do at least the following:
> > >
> > > 1. Remove @Deprecated from old API (because it strange to have one
> > > deprecated API and second experimental API)
> > > 2. Add @IgniteExperimetnal to new API (because... see item. 1)
> > > 3. Do not merge IGNITE-12553 (because it adds new public interface
> > > that 100% will be changed)
> > >
> > > ideally we should also:
> > >
> > > 4. Add metrics that available only via new API to the old API (because
> > > otherwise we force user interact with both API's)
> > >
> > > On Thu, Jan 30, 2020 at 12:35 PM Alexey Goncharuk
> > > wrote:
> > >>
> > >> Folks,
> > >>
> > >> I tried to re-read the whole thread and honestly got lost at the end
> :)
> > Do
> > >> we have a consensus (if yes, what are the steps?) or should we have a
> > call
> > >> as Maxim suggested?
> > >>
> > >> I think it is in our best interest to get this agreed upon fast to
> > release
> > >> AI 2.8 soon.
> >
> >
>
--
Best regards,
Andrey V. Mashenkov
Re: Internal classes are exposed in public API
чт, 30 янв. 2020 г. в 15:18, Anton Vinogradov : > We should have all the options. > > Deprecated -> will be removed at major release (because this way is slow or > overcomplicated) but still be used now, > Not only does it mean that the API should be removed in a major release, but there is a stable counterpart that can be used instead. > Annotation is absent -> just a stable (proper) API, > Experimental -> not stable yet or partially implemented or in fact an > experimental > Looks like the formal vote is the only way to go now.
Re: Internal classes are exposed in public API
>> . Remove @Deprecated from old API (because it strange to have one
>> deprecated API and second experimental API)
>> 2. Add @IgniteExperimetnal to new API (because... see item. 1)
-1
We should have all the options.
Deprecated -> will be removed at major release (because this way is slow or
overcomplicated) but still be used now,
Annotation is absent -> just a stable (proper) API,
Experimental -> not stable yet or partially implemented or in fact an
experimental
On Thu, Jan 30, 2020 at 2:47 PM Maxim Muzafarov wrote:
> Igniters,
>
>
> How about to start a vote (formal) over the whole community to choose
> between these two options we have? With one caveat that veto votes are not
> applicable to this discussion. Let the whole community decide what
> direction we should move on.
>
> On Thu, 30 Jan 2020 at 14:34, Nikolay Izhikov wrote:
>
> > Hello.
> >
> > > 2. Add @IgniteExperimetnal to new API (because... see item. 1
> >
> > +1
> >
> > > 1. Remove @Deprecated from old API (because it strange to have one
> > deprecated API and second experimental API)
> >
> > -1
> >
> > I propose to update deprecation message and provide metric name for each
> > deprecated method.
> >
> > > @deprecated Use {@link JmxMetricExporterSPI} instead. Name of the
> metric
> > «io.dataregion.pageCount»
> >
> > Andrey.
> >
> > We doesn’t come to an agreement that API should be changed.
> > We should discuss the design of the Metric API and your proposals for it
> > in another thread.
> >
> > Please, avoid arguments like «this API will be 100% changed» in this
> > discussion.
> >
> > > 30 янв. 2020 г., в 14:21, Andrey Gura написал(а):
> > >
> > > From my point of view we still don't have consensus.
> > >
> > > I think we should do at least the following:
> > >
> > > 1. Remove @Deprecated from old API (because it strange to have one
> > > deprecated API and second experimental API)
> > > 2. Add @IgniteExperimetnal to new API (because... see item. 1)
> > > 3. Do not merge IGNITE-12553 (because it adds new public interface
> > > that 100% will be changed)
> > >
> > > ideally we should also:
> > >
> > > 4. Add metrics that available only via new API to the old API (because
> > > otherwise we force user interact with both API's)
> > >
> > > On Thu, Jan 30, 2020 at 12:35 PM Alexey Goncharuk
> > > wrote:
> > >>
> > >> Folks,
> > >>
> > >> I tried to re-read the whole thread and honestly got lost at the end
> :)
> > Do
> > >> we have a consensus (if yes, what are the steps?) or should we have a
> > call
> > >> as Maxim suggested?
> > >>
> > >> I think it is in our best interest to get this agreed upon fast to
> > release
> > >> AI 2.8 soon.
> >
> >
>
Re: Internal classes are exposed in public API
Hello.
> 2. Add @IgniteExperimetnal to new API (because... see item. 1
+1
> 1. Remove @Deprecated from old API (because it strange to have one deprecated
> API and second experimental API)
-1
I propose to update deprecation message and provide metric name for each
deprecated method.
> @deprecated Use {@link JmxMetricExporterSPI} instead. Name of the metric
> «io.dataregion.pageCount»
Andrey.
We doesn’t come to an agreement that API should be changed.
We should discuss the design of the Metric API and your proposals for it in
another thread.
Please, avoid arguments like «this API will be 100% changed» in this discussion.
> 30 янв. 2020 г., в 14:21, Andrey Gura написал(а):
>
> From my point of view we still don't have consensus.
>
> I think we should do at least the following:
>
> 1. Remove @Deprecated from old API (because it strange to have one
> deprecated API and second experimental API)
> 2. Add @IgniteExperimetnal to new API (because... see item. 1)
> 3. Do not merge IGNITE-12553 (because it adds new public interface
> that 100% will be changed)
>
> ideally we should also:
>
> 4. Add metrics that available only via new API to the old API (because
> otherwise we force user interact with both API's)
>
> On Thu, Jan 30, 2020 at 12:35 PM Alexey Goncharuk
> wrote:
>>
>> Folks,
>>
>> I tried to re-read the whole thread and honestly got lost at the end :) Do
>> we have a consensus (if yes, what are the steps?) or should we have a call
>> as Maxim suggested?
>>
>> I think it is in our best interest to get this agreed upon fast to release
>> AI 2.8 soon.
Re: Internal classes are exposed in public API
>From my point of view we still don't have consensus. I think we should do at least the following: 1. Remove @Deprecated from old API (because it strange to have one deprecated API and second experimental API) 2. Add @IgniteExperimetnal to new API (because... see item. 1) 3. Do not merge IGNITE-12553 (because it adds new public interface that 100% will be changed) ideally we should also: 4. Add metrics that available only via new API to the old API (because otherwise we force user interact with both API's) On Thu, Jan 30, 2020 at 12:35 PM Alexey Goncharuk wrote: > > Folks, > > I tried to re-read the whole thread and honestly got lost at the end :) Do > we have a consensus (if yes, what are the steps?) or should we have a call > as Maxim suggested? > > I think it is in our best interest to get this agreed upon fast to release > AI 2.8 soon.
Re: Internal classes are exposed in public API
Folks, I tried to re-read the whole thread and honestly got lost at the end :) Do we have a consensus (if yes, what are the steps?) or should we have a call as Maxim suggested? I think it is in our best interest to get this agreed upon fast to release AI 2.8 soon.
Re: Internal classes are exposed in public API
t about API which is known will be >>>>> changed. >>>>>>> Second argument: why we should add this experimental API to the >>>>>>> already existing experimental API? Just to making API more >>>>>>> experimental? As I told already it is commit for commit and doesn't >>>>>>> bring any value but brings some inconvenience to me (e.g. merge >>>>>>> problems). >>>>>>> >>>>>>> BTW, does it exist issue about marking IEP-35 API's as experimental? >>>>>>> >>>>>>> On Thu, Jan 23, 2020 at 8:33 PM Николай Ижиков >>>>> wrote: >>>>>>>> >>>>>>>>> You want say didn't discuss? >>>>>>>> >>>>>>>> Yes. >>>>>>>> >>>>>>>>> Secondly, yes it took a lot of time due to a lot of changes. Is it >>>>> problem? >>>>>>>> >>>>>>>> No, of course. >>>>>>>> My point - that your contribution that took a long time, already, >>>>> can’t be an argument to postpone creation of the API in the current >>>>> release. >>>>>>>> >>>>>>>> >>>>>>>>> 23 янв. 2020 г., в 18:11, Andrey Gura написал(а): >>>>>>>>> >>>>>>>>>> We don’t discuss your changes and your proposals for the Metric API. >>>>>>>>> >>>>>>>>> You want say didn't discuss? Actually we did it [1] but you told ok >>>>>>>>> let's see code :) >>>>>>>>> >>>>>>>>>> So I don’t think we can make the existence of some PR is an >>>>> argument to introduce(or not introduce) this facade. >>>>>>>>> >>>>>>>>> You definitely right in case of competition development. But I think >>>>>>>>> that collaborative development is more effective. Isn't it? >>>>>>>>> >>>>>>>>>> Moreover, As far as I know, you developing changes for the Metric >>>>> API for 5 months without public discussion, for now. Let's start it. >>>>>>>>> >>>>>>>>> Firsty, with discussion. See above. >>>>>>>>> Secondly, yes it took a lot of time due to a lot of changes. Is it >>>>> problem? >>>>>>>>> >>>>>>>>>> Let’s do the following: >>>>>>>>>> 1. Review `IgniteMetric` facade and introduce it to the users as an >>>>> experimental API. >>>>>>>>> >>>>>>>>> It just doesn't make sense. Just commit for commit. >>>>>>>>> >>>>>>>>>> 2. Discuss your proposal to the Metric API in the separate thread >>>>> you are referencing. >>>>>>>>> >>>>>>>>> You a re welcome to thread [1] again. But please, take into account. >>>>>>>>> because discussion is postponed by you it's late enough to discuss >>>>>>>>> proposed solution. But of course I'll answer on your questions and >>>>>>>>> will be glade to your comments and suggestions. >>>>>>>>> >>>>>>>>>> I think we should start a discussion from the simplified >>>>> explanation of: >>>>>>>>>> 1. API intentions - What we want to gain with it? What is our focus? >>>>>>>>>> 2. Simple, minimal example of API main interfaces and desired >>>>> usages. >>>>>>>>> >>>>>>>>> All this already described at [1]. You also can take a look on PR >>>>> (see >>>>>>>>> MetricSource implementations, there are complex and simple ones). >>>>>>>>> >>>>>>>>> [1] >>>>> http://apache-ignite-developers.2346864.n4.nabble.com/IEP-35-Metrics-management-in-Ignite-tp43243.html >>>>>>>>> >>>>>>>>> On Thu, Jan 23, 2020 at 5:42 PM Николай Ижиков >>>>> wrote: >>>>>>>>>> >>>>>>>>>> Andrey. >>>>>>>>>> >>&g
Re: Internal classes are exposed in public API
t;>>> Secondly, yes it took a lot of time due to a lot of changes. Is it > > >> problem? > > >>>>> > > >>>>> No, of course. > > >>>>> My point - that your contribution that took a long time, already, > > >> can’t be an argument to postpone creation of the API in the current > > >> release. > > >>>>> > > >>>>> > > >>>>>> 23 янв. 2020 г., в 18:11, Andrey Gura написал(а): > > >>>>>> > > >>>>>>> We don’t discuss your changes and your proposals for the Metric API. > > >>>>>> > > >>>>>> You want say didn't discuss? Actually we did it [1] but you told ok > > >>>>>> let's see code :) > > >>>>>> > > >>>>>>> So I don’t think we can make the existence of some PR is an > > >> argument to introduce(or not introduce) this facade. > > >>>>>> > > >>>>>> You definitely right in case of competition development. But I think > > >>>>>> that collaborative development is more effective. Isn't it? > > >>>>>> > > >>>>>>> Moreover, As far as I know, you developing changes for the Metric > > >> API for 5 months without public discussion, for now. Let's start it. > > >>>>>> > > >>>>>> Firsty, with discussion. See above. > > >>>>>> Secondly, yes it took a lot of time due to a lot of changes. Is it > > >> problem? > > >>>>>> > > >>>>>>> Let’s do the following: > > >>>>>>> 1. Review `IgniteMetric` facade and introduce it to the users as an > > >> experimental API. > > >>>>>> > > >>>>>> It just doesn't make sense. Just commit for commit. > > >>>>>> > > >>>>>>> 2. Discuss your proposal to the Metric API in the separate thread > > >> you are referencing. > > >>>>>> > > >>>>>> You a re welcome to thread [1] again. But please, take into account. > > >>>>>> because discussion is postponed by you it's late enough to discuss > > >>>>>> proposed solution. But of course I'll answer on your questions and > > >>>>>> will be glade to your comments and suggestions. > > >>>>>> > > >>>>>>> I think we should start a discussion from the simplified > > >> explanation of: > > >>>>>>> 1. API intentions - What we want to gain with it? What is our focus? > > >>>>>>> 2. Simple, minimal example of API main interfaces and desired > > >> usages. > > >>>>>> > > >>>>>> All this already described at [1]. You also can take a look on PR > > >> (see > > >>>>>> MetricSource implementations, there are complex and simple ones). > > >>>>>> > > >>>>>> [1] > > >> http://apache-ignite-developers.2346864.n4.nabble.com/IEP-35-Metrics-management-in-Ignite-tp43243.html > > >>>>>> > > >>>>>> On Thu, Jan 23, 2020 at 5:42 PM Николай Ижиков > > >> wrote: > > >>>>>>> > > >>>>>>> Andrey. > > >>>>>>> > > >>>>>>>> IGNITE-11927 implementation so your changes are incompatible with > > >> my changes > > >>>>>>> > > >>>>>>> We don’t discuss your changes and your proposals for the Metric API. > > >>>>>>> So I don’t think we can make the existence of some PR is an > > >> argument to introduce(or not introduce) this facade. > > >>>>>>> Moreover, As far as I know, you developing changes for the Metric > > >> API for 5 months without public discussion, for now. Let's start it. > > >>>>>>> > > >>>>>>> Let’s do the following: > > >>>>>>> > > >>>>>>> 1. Review `IgniteMetric` facade and introduce it to the users as an > > >> experimental API. > > >>>>>>> > > >>>>>>> 2. Discuss your proposal to the Metric API in the separate thread > > >> you are
Re: Internal classes are exposed in public API
sages. > >>>>>> > >>>>>> All this already described at [1]. You also can take a look on PR > >> (see > >>>>>> MetricSource implementations, there are complex and simple ones). > >>>>>> > >>>>>> [1] > >> http://apache-ignite-developers.2346864.n4.nabble.com/IEP-35-Metrics-management-in-Ignite-tp43243.html > >>>>>> > >>>>>> On Thu, Jan 23, 2020 at 5:42 PM Николай Ижиков > >> wrote: > >>>>>>> > >>>>>>> Andrey. > >>>>>>> > >>>>>>>> IGNITE-11927 implementation so your changes are incompatible with > >> my changes > >>>>>>> > >>>>>>> We don’t discuss your changes and your proposals for the Metric API. > >>>>>>> So I don’t think we can make the existence of some PR is an > >> argument to introduce(or not introduce) this facade. > >>>>>>> Moreover, As far as I know, you developing changes for the Metric > >> API for 5 months without public discussion, for now. Let's start it. > >>>>>>> > >>>>>>> Let’s do the following: > >>>>>>> > >>>>>>> 1. Review `IgniteMetric` facade and introduce it to the users as an > >> experimental API. > >>>>>>> > >>>>>>> 2. Discuss your proposal to the Metric API in the separate thread > >> you are referencing. > >>>>>>> > >>>>>>> I think we should start a discussion from the simplified > >> explanation of: > >>>>>>> > >>>>>>> 1. API intentions - What we want to gain with it? What is our focus? > >>>>>>> 2. Simple, minimal example of API main interfaces and desired > >> usages. > >>>>>>> > >>>>>>> This will help to the community and me personally better understand > >> your idea and share feedback. > >>>>>>> > >>>>>>> Thanks. > >>>>>>> > >>>>>>>> 23 янв. 2020 г., в 17:15, Andrey Gura > >> написал(а): > >>>>>>>> > >>>>>>>> Nikolay, > >>>>>>>> > >>>>>>>> as I wrote early in this thread API significantly changed during > >>>>>>>> IGNITE-11927 implementation so your changes are incompatible with > >> my > >>>>>>>> changes. > >>>>>>>> > >>>>>>>> ReadOnlyMetricRegistry will be removed at all (still exists in a > >>>>>>>> couple of places where it uses). > >>>>>>>> > >>>>>>>> Also I don't want to introduce IgniteMetric facade in this rush. In > >>>>>>>> current implementation this interface just provides access to the > >>>>>>>> ReadOnlyMetricManager instance (which will be removed) but it is > >> not > >>>>>>>> enough. > >>>>>>>> > >>>>>>>> I agree with Denis. We should mark current API as experimental and > >>>>>>>> continue IEP-35 development. See my process proposal [1] described > >>>>>>>> early this thread. We can release Apache Ignite 2.8.1 specially for > >>>>>>>> this changes. > >>>>>>>> Public APIs require deeper thinking in order to provide > >> comprehensive, > >>>>>>>> consistent and convenient way of metrics management for end users. > >>>>>>>> > >>>>>>>> Let's add IgniteExperimental, release 2.8 and finish metrics > >> related > >>>>>>>> issues after it. > >>>>>>>> > >>>>>>>> [1] > >> http://apache-ignite-developers.2346864.n4.nabble.com/Internal-classes-are-exposed-in-public-API-tp45146p45185.html > >>>>>>>> > >>>>>>>> > >>>>>>>> On Wed, Jan 22, 2020 at 5:21 PM Николай Ижиков < > >> nizhi...@apache.org> wrote: > >>>>>>>>> > >>>>>>>>> Hello, Igniters. > >>>>>>>>> > >>>>>>>
Re: Internal classes are exposed in public API
don’t discuss your changes and your proposals for the Metric API. > >>>>>>> So I don’t think we can make the existence of some PR is an > >> argument to introduce(or not introduce) this facade. > >>>>>>> Moreover, As far as I know, you developing changes for the Metric > >> API for 5 months without public discussion, for now. Let's start it. > >>>>>>> > >>>>>>> Let’s do the following: > >>>>>>> > >>>>>>> 1. Review `IgniteMetric` facade and introduce it to the users as an > >> experimental API. > >>>>>>> > >>>>>>> 2. Discuss your proposal to the Metric API in the separate thread > >> you are referencing. > >>>>>>> > >>>>>>> I think we should start a discussion from the simplified > >> explanation of: > >>>>>>> > >>>>>>> 1. API intentions - What we want to gain with it? What is our focus? > >>>>>>> 2. Simple, minimal example of API main interfaces and desired > >> usages. > >>>>>>> > >>>>>>> This will help to the community and me personally better understand > >> your idea and share feedback. > >>>>>>> > >>>>>>> Thanks. > >>>>>>> > >>>>>>>> 23 янв. 2020 г., в 17:15, Andrey Gura > >> написал(а): > >>>>>>>> > >>>>>>>> Nikolay, > >>>>>>>> > >>>>>>>> as I wrote early in this thread API significantly changed during > >>>>>>>> IGNITE-11927 implementation so your changes are incompatible with > >> my > >>>>>>>> changes. > >>>>>>>> > >>>>>>>> ReadOnlyMetricRegistry will be removed at all (still exists in a > >>>>>>>> couple of places where it uses). > >>>>>>>> > >>>>>>>> Also I don't want to introduce IgniteMetric facade in this rush. In > >>>>>>>> current implementation this interface just provides access to the > >>>>>>>> ReadOnlyMetricManager instance (which will be removed) but it is > >> not > >>>>>>>> enough. > >>>>>>>> > >>>>>>>> I agree with Denis. We should mark current API as experimental and > >>>>>>>> continue IEP-35 development. See my process proposal [1] described > >>>>>>>> early this thread. We can release Apache Ignite 2.8.1 specially for > >>>>>>>> this changes. > >>>>>>>> Public APIs require deeper thinking in order to provide > >> comprehensive, > >>>>>>>> consistent and convenient way of metrics management for end users. > >>>>>>>> > >>>>>>>> Let's add IgniteExperimental, release 2.8 and finish metrics > >> related > >>>>>>>> issues after it. > >>>>>>>> > >>>>>>>> [1] > >> http://apache-ignite-developers.2346864.n4.nabble.com/Internal-classes-are-exposed-in-public-API-tp45146p45185.html > >>>>>>>> > >>>>>>>> > >>>>>>>> On Wed, Jan 22, 2020 at 5:21 PM Николай Ижиков < > >> nizhi...@apache.org> wrote: > >>>>>>>>> > >>>>>>>>> Hello, Igniters. > >>>>>>>>> > >>>>>>>>> * IGNITE-12552: Move ReadOnlyMetricRegistry to public API merged > >> to the master and cherry-picked to the 2.8. > >>>>>>>>> So the main issue with the Metric API solved. > >>>>>>>>> > >>>>>>>>> * I raised the PR [1] to fix second issue with the new Metric > >> API: absence of the public Java API to get metrics. > >>>>>>>>> This PR introduces the following changes: > >>>>>>>>> > >>>>>>>>> 1. IgniteMetric interface created: it provides Java API to access > >> Ignite metrics created with the new Metric API. > >>>>>>>>> > >>>>>>>>> ``` > >>>>>>>>> public interface IgniteMetric extends &
Re: Internal classes are exposed in public API
st issue about marking IEP-35 API's as experimental? >>>> >>>> On Thu, Jan 23, 2020 at 8:33 PM Николай Ижиков >> wrote: >>>>> >>>>>> You want say didn't discuss? >>>>> >>>>> Yes. >>>>> >>>>>> Secondly, yes it took a lot of time due to a lot of changes. Is it >> problem? >>>>> >>>>> No, of course. >>>>> My point - that your contribution that took a long time, already, >> can’t be an argument to postpone creation of the API in the current release. >>>>> >>>>> >>>>>> 23 янв. 2020 г., в 18:11, Andrey Gura написал(а): >>>>>> >>>>>>> We don’t discuss your changes and your proposals for the Metric API. >>>>>> >>>>>> You want say didn't discuss? Actually we did it [1] but you told ok >>>>>> let's see code :) >>>>>> >>>>>>> So I don’t think we can make the existence of some PR is an >> argument to introduce(or not introduce) this facade. >>>>>> >>>>>> You definitely right in case of competition development. But I think >>>>>> that collaborative development is more effective. Isn't it? >>>>>> >>>>>>> Moreover, As far as I know, you developing changes for the Metric >> API for 5 months without public discussion, for now. Let's start it. >>>>>> >>>>>> Firsty, with discussion. See above. >>>>>> Secondly, yes it took a lot of time due to a lot of changes. Is it >> problem? >>>>>> >>>>>>> Let’s do the following: >>>>>>> 1. Review `IgniteMetric` facade and introduce it to the users as an >> experimental API. >>>>>> >>>>>> It just doesn't make sense. Just commit for commit. >>>>>> >>>>>>> 2. Discuss your proposal to the Metric API in the separate thread >> you are referencing. >>>>>> >>>>>> You a re welcome to thread [1] again. But please, take into account. >>>>>> because discussion is postponed by you it's late enough to discuss >>>>>> proposed solution. But of course I'll answer on your questions and >>>>>> will be glade to your comments and suggestions. >>>>>> >>>>>>> I think we should start a discussion from the simplified >> explanation of: >>>>>>> 1. API intentions - What we want to gain with it? What is our focus? >>>>>>> 2. Simple, minimal example of API main interfaces and desired >> usages. >>>>>> >>>>>> All this already described at [1]. You also can take a look on PR >> (see >>>>>> MetricSource implementations, there are complex and simple ones). >>>>>> >>>>>> [1] >> http://apache-ignite-developers.2346864.n4.nabble.com/IEP-35-Metrics-management-in-Ignite-tp43243.html >>>>>> >>>>>> On Thu, Jan 23, 2020 at 5:42 PM Николай Ижиков >> wrote: >>>>>>> >>>>>>> Andrey. >>>>>>> >>>>>>>> IGNITE-11927 implementation so your changes are incompatible with >> my changes >>>>>>> >>>>>>> We don’t discuss your changes and your proposals for the Metric API. >>>>>>> So I don’t think we can make the existence of some PR is an >> argument to introduce(or not introduce) this facade. >>>>>>> Moreover, As far as I know, you developing changes for the Metric >> API for 5 months without public discussion, for now. Let's start it. >>>>>>> >>>>>>> Let’s do the following: >>>>>>> >>>>>>> 1. Review `IgniteMetric` facade and introduce it to the users as an >> experimental API. >>>>>>> >>>>>>> 2. Discuss your proposal to the Metric API in the separate thread >> you are referencing. >>>>>>> >>>>>>> I think we should start a discussion from the simplified >> explanation of: >>>>>>> >>>>>>> 1. API intentions - What we want to gain with it? What is our focus? >>>>>>> 2. Simple, minimal example of API main interfaces and desired >> usages. >>>>>>> >>>>>>> Thi
Re: Internal classes are exposed in public API
;>> > > >>> Firsty, with discussion. See above. > > >>> Secondly, yes it took a lot of time due to a lot of changes. Is it > problem? > > >>> > > >>>> Let’s do the following: > > >>>> 1. Review `IgniteMetric` facade and introduce it to the users as an > experimental API. > > >>> > > >>> It just doesn't make sense. Just commit for commit. > > >>> > > >>>> 2. Discuss your proposal to the Metric API in the separate thread > you are referencing. > > >>> > > >>> You a re welcome to thread [1] again. But please, take into account. > > >>> because discussion is postponed by you it's late enough to discuss > > >>> proposed solution. But of course I'll answer on your questions and > > >>> will be glade to your comments and suggestions. > > >>> > > >>>> I think we should start a discussion from the simplified > explanation of: > > >>>> 1. API intentions - What we want to gain with it? What is our focus? > > >>>> 2. Simple, minimal example of API main interfaces and desired > usages. > > >>> > > >>> All this already described at [1]. You also can take a look on PR > (see > > >>> MetricSource implementations, there are complex and simple ones). > > >>> > > >>> [1] > http://apache-ignite-developers.2346864.n4.nabble.com/IEP-35-Metrics-management-in-Ignite-tp43243.html > > >>> > > >>> On Thu, Jan 23, 2020 at 5:42 PM Николай Ижиков > wrote: > > >>>> > > >>>> Andrey. > > >>>> > > >>>>> IGNITE-11927 implementation so your changes are incompatible with > my changes > > >>>> > > >>>> We don’t discuss your changes and your proposals for the Metric API. > > >>>> So I don’t think we can make the existence of some PR is an > argument to introduce(or not introduce) this facade. > > >>>> Moreover, As far as I know, you developing changes for the Metric > API for 5 months without public discussion, for now. Let's start it. > > >>>> > > >>>> Let’s do the following: > > >>>> > > >>>> 1. Review `IgniteMetric` facade and introduce it to the users as an > experimental API. > > >>>> > > >>>> 2. Discuss your proposal to the Metric API in the separate thread > you are referencing. > > >>>> > > >>>> I think we should start a discussion from the simplified > explanation of: > > >>>> > > >>>> 1. API intentions - What we want to gain with it? What is our focus? > > >>>> 2. Simple, minimal example of API main interfaces and desired > usages. > > >>>> > > >>>> This will help to the community and me personally better understand > your idea and share feedback. > > >>>> > > >>>> Thanks. > > >>>> > > >>>>> 23 янв. 2020 г., в 17:15, Andrey Gura > написал(а): > > >>>>> > > >>>>> Nikolay, > > >>>>> > > >>>>> as I wrote early in this thread API significantly changed during > > >>>>> IGNITE-11927 implementation so your changes are incompatible with > my > > >>>>> changes. > > >>>>> > > >>>>> ReadOnlyMetricRegistry will be removed at all (still exists in a > > >>>>> couple of places where it uses). > > >>>>> > > >>>>> Also I don't want to introduce IgniteMetric facade in this rush. In > > >>>>> current implementation this interface just provides access to the > > >>>>> ReadOnlyMetricManager instance (which will be removed) but it is > not > > >>>>> enough. > > >>>>> > > >>>>> I agree with Denis. We should mark current API as experimental and > > >>>>> continue IEP-35 development. See my process proposal [1] described > > >>>>> early this thread. We can release Apache Ignite 2.8.1 specially for > > >>>>> this changes. > > >>>>> Public APIs require deeper thinking in order to provide > comprehensive, > > >>>>> consistent and convenient way of metrics management for end users. > > >>>>> > > >
Re: Internal classes are exposed in public API
s-management-in-Ignite-tp43243.html
> >>>
> >>> On Thu, Jan 23, 2020 at 5:42 PM Николай Ижиков
> >>> wrote:
> >>>>
> >>>> Andrey.
> >>>>
> >>>>> IGNITE-11927 implementation so your changes are incompatible with my
> >>>>> changes
> >>>>
> >>>> We don’t discuss your changes and your proposals for the Metric API.
> >>>> So I don’t think we can make the existence of some PR is an argument to
> >>>> introduce(or not introduce) this facade.
> >>>> Moreover, As far as I know, you developing changes for the Metric API
> >>>> for 5 months without public discussion, for now. Let's start it.
> >>>>
> >>>> Let’s do the following:
> >>>>
> >>>> 1. Review `IgniteMetric` facade and introduce it to the users as an
> >>>> experimental API.
> >>>>
> >>>> 2. Discuss your proposal to the Metric API in the separate thread you
> >>>> are referencing.
> >>>>
> >>>> I think we should start a discussion from the simplified explanation of:
> >>>>
> >>>> 1. API intentions - What we want to gain with it? What is our focus?
> >>>> 2. Simple, minimal example of API main interfaces and desired usages.
> >>>>
> >>>> This will help to the community and me personally better understand your
> >>>> idea and share feedback.
> >>>>
> >>>> Thanks.
> >>>>
> >>>>> 23 янв. 2020 г., в 17:15, Andrey Gura написал(а):
> >>>>>
> >>>>> Nikolay,
> >>>>>
> >>>>> as I wrote early in this thread API significantly changed during
> >>>>> IGNITE-11927 implementation so your changes are incompatible with my
> >>>>> changes.
> >>>>>
> >>>>> ReadOnlyMetricRegistry will be removed at all (still exists in a
> >>>>> couple of places where it uses).
> >>>>>
> >>>>> Also I don't want to introduce IgniteMetric facade in this rush. In
> >>>>> current implementation this interface just provides access to the
> >>>>> ReadOnlyMetricManager instance (which will be removed) but it is not
> >>>>> enough.
> >>>>>
> >>>>> I agree with Denis. We should mark current API as experimental and
> >>>>> continue IEP-35 development. See my process proposal [1] described
> >>>>> early this thread. We can release Apache Ignite 2.8.1 specially for
> >>>>> this changes.
> >>>>> Public APIs require deeper thinking in order to provide comprehensive,
> >>>>> consistent and convenient way of metrics management for end users.
> >>>>>
> >>>>> Let's add IgniteExperimental, release 2.8 and finish metrics related
> >>>>> issues after it.
> >>>>>
> >>>>> [1]
> >>>>> http://apache-ignite-developers.2346864.n4.nabble.com/Internal-classes-are-exposed-in-public-API-tp45146p45185.html
> >>>>>
> >>>>>
> >>>>> On Wed, Jan 22, 2020 at 5:21 PM Николай Ижиков
> >>>>> wrote:
> >>>>>>
> >>>>>> Hello, Igniters.
> >>>>>>
> >>>>>> * IGNITE-12552: Move ReadOnlyMetricRegistry to public API merged to
> >>>>>> the master and cherry-picked to the 2.8.
> >>>>>> So the main issue with the Metric API solved.
> >>>>>>
> >>>>>> * I raised the PR [1] to fix second issue with the new Metric API:
> >>>>>> absence of the public Java API to get metrics.
> >>>>>> This PR introduces the following changes:
> >>>>>>
> >>>>>> 1. IgniteMetric interface created: it provides Java API to access
> >>>>>> Ignite metrics created with the new Metric API.
> >>>>>>
> >>>>>> ```
> >>>>>> public interface IgniteMetric extends Iterable
> >>>>>> {
> >>>>>> @Nullable ReadOnlyMetricRegistry registry(String name);
> >>>>>> }
> >>>>>> ```
> >>>>>>
> >>>>>> 2. All deprecation javadocs regarding metrics now reference to the
>
Re: Internal classes are exposed in public API
;>
>>>> 1. Review `IgniteMetric` facade and introduce it to the users as an
>>>> experimental API.
>>>>
>>>> 2. Discuss your proposal to the Metric API in the separate thread you are
>>>> referencing.
>>>>
>>>> I think we should start a discussion from the simplified explanation of:
>>>>
>>>> 1. API intentions - What we want to gain with it? What is our focus?
>>>> 2. Simple, minimal example of API main interfaces and desired usages.
>>>>
>>>> This will help to the community and me personally better understand your
>>>> idea and share feedback.
>>>>
>>>> Thanks.
>>>>
>>>>> 23 янв. 2020 г., в 17:15, Andrey Gura написал(а):
>>>>>
>>>>> Nikolay,
>>>>>
>>>>> as I wrote early in this thread API significantly changed during
>>>>> IGNITE-11927 implementation so your changes are incompatible with my
>>>>> changes.
>>>>>
>>>>> ReadOnlyMetricRegistry will be removed at all (still exists in a
>>>>> couple of places where it uses).
>>>>>
>>>>> Also I don't want to introduce IgniteMetric facade in this rush. In
>>>>> current implementation this interface just provides access to the
>>>>> ReadOnlyMetricManager instance (which will be removed) but it is not
>>>>> enough.
>>>>>
>>>>> I agree with Denis. We should mark current API as experimental and
>>>>> continue IEP-35 development. See my process proposal [1] described
>>>>> early this thread. We can release Apache Ignite 2.8.1 specially for
>>>>> this changes.
>>>>> Public APIs require deeper thinking in order to provide comprehensive,
>>>>> consistent and convenient way of metrics management for end users.
>>>>>
>>>>> Let's add IgniteExperimental, release 2.8 and finish metrics related
>>>>> issues after it.
>>>>>
>>>>> [1]
>>>>> http://apache-ignite-developers.2346864.n4.nabble.com/Internal-classes-are-exposed-in-public-API-tp45146p45185.html
>>>>>
>>>>>
>>>>> On Wed, Jan 22, 2020 at 5:21 PM Николай Ижиков
>>>>> wrote:
>>>>>>
>>>>>> Hello, Igniters.
>>>>>>
>>>>>> * IGNITE-12552: Move ReadOnlyMetricRegistry to public API merged to the
>>>>>> master and cherry-picked to the 2.8.
>>>>>> So the main issue with the Metric API solved.
>>>>>>
>>>>>> * I raised the PR [1] to fix second issue with the new Metric API:
>>>>>> absence of the public Java API to get metrics.
>>>>>> This PR introduces the following changes:
>>>>>>
>>>>>> 1. IgniteMetric interface created: it provides Java API to access Ignite
>>>>>> metrics created with the new Metric API.
>>>>>>
>>>>>> ```
>>>>>> public interface IgniteMetric extends Iterable {
>>>>>> @Nullable ReadOnlyMetricRegistry registry(String name);
>>>>>> }
>>>>>> ```
>>>>>>
>>>>>> 2. All deprecation javadocs regarding metrics now reference to the
>>>>>> public IgniteMetric instead of internal GridMetricManager:
>>>>>>
>>>>>>> @deprecated Use {@link IgniteMetric} instead.
>>>>>>
>>>>>> 3. Tests refactored to use IgniteMetric instead of GridMetricManager
>>>>>> when possible.
>>>>>>
>>>>>> Please, review.
>>>>>>
>>>>>> [1] https://github.com/apache/ignite/pull/7283
>>>>>>
>>>>>>> 21 янв. 2020 г., в 17:51, Николай Ижиков
>>>>>>> написал(а):
>>>>>>>
>>>>>>> Hello, Igniters.
>>>>>>>
>>>>>>> Alexey approved my PR [1] regarding fixing public API for metric
>>>>>>> exporters.
>>>>>>> I’m waiting for a bot visa and merge this PR.
>>>>>>>
>>>>>>> As we discussed, the metrics API will be marked with IgniteExperimental
>>>>>>> annotation.
>>>>>>>
>>>>>>> If anyone has any objection to this plan, please pro
Re: Internal classes are exposed in public API
it uses).
> >>>
> >>> Also I don't want to introduce IgniteMetric facade in this rush. In
> >>> current implementation this interface just provides access to the
> >>> ReadOnlyMetricManager instance (which will be removed) but it is not
> >>> enough.
> >>>
> >>> I agree with Denis. We should mark current API as experimental and
> >>> continue IEP-35 development. See my process proposal [1] described
> >>> early this thread. We can release Apache Ignite 2.8.1 specially for
> >>> this changes.
> >>> Public APIs require deeper thinking in order to provide comprehensive,
> >>> consistent and convenient way of metrics management for end users.
> >>>
> >>> Let's add IgniteExperimental, release 2.8 and finish metrics related
> >>> issues after it.
> >>>
> >>> [1]
> >>> http://apache-ignite-developers.2346864.n4.nabble.com/Internal-classes-are-exposed-in-public-API-tp45146p45185.html
> >>>
> >>>
> >>> On Wed, Jan 22, 2020 at 5:21 PM Николай Ижиков
> >>> wrote:
> >>>>
> >>>> Hello, Igniters.
> >>>>
> >>>> * IGNITE-12552: Move ReadOnlyMetricRegistry to public API merged to the
> >>>> master and cherry-picked to the 2.8.
> >>>> So the main issue with the Metric API solved.
> >>>>
> >>>> * I raised the PR [1] to fix second issue with the new Metric API:
> >>>> absence of the public Java API to get metrics.
> >>>> This PR introduces the following changes:
> >>>>
> >>>> 1. IgniteMetric interface created: it provides Java API to access Ignite
> >>>> metrics created with the new Metric API.
> >>>>
> >>>> ```
> >>>> public interface IgniteMetric extends Iterable {
> >>>> @Nullable ReadOnlyMetricRegistry registry(String name);
> >>>> }
> >>>> ```
> >>>>
> >>>> 2. All deprecation javadocs regarding metrics now reference to the
> >>>> public IgniteMetric instead of internal GridMetricManager:
> >>>>
> >>>>> @deprecated Use {@link IgniteMetric} instead.
> >>>>
> >>>> 3. Tests refactored to use IgniteMetric instead of GridMetricManager
> >>>> when possible.
> >>>>
> >>>> Please, review.
> >>>>
> >>>> [1] https://github.com/apache/ignite/pull/7283
> >>>>
> >>>>> 21 янв. 2020 г., в 17:51, Николай Ижиков
> >>>>> написал(а):
> >>>>>
> >>>>> Hello, Igniters.
> >>>>>
> >>>>> Alexey approved my PR [1] regarding fixing public API for metric
> >>>>> exporters.
> >>>>> I’m waiting for a bot visa and merge this PR.
> >>>>>
> >>>>> As we discussed, the metrics API will be marked with IgniteExperimental
> >>>>> annotation.
> >>>>>
> >>>>> If anyone has any objection to this plan, please provide your feedback.
> >>>>>
> >>>>> [1] https://github.com/apache/ignite/pull/7269
> >>>>>
> >>>>>> 21 янв. 2020 г., в 13:45, Николай Ижиков
> >>>>>> написал(а):
> >>>>>>
> >>>>>> Thanks, for the review Alexey.
> >>>>>>
> >>>>>> I will fix your comment and try to implement Monitoring facade,
> >>>>>> shortly.
> >>>>>>
> >>>>>>> 21 янв. 2020 г., в 13:32, Alexey Goncharuk
> >>>>>>> написал(а):
> >>>>>>>
> >>>>>>> Nikolay,
> >>>>>>>
> >>>>>>> I left a single comment in the PR about the histogram metric. I think
> >>>>>>> the
> >>>>>>> API looks much cleaner now.
> >>>>>>>
> >>>>>>> I will take care of the @IgniteExperimental annotation.
> >>>>>>>
> >>>>>>> пн, 20 янв. 2020 г. в 20:55, Николай Ижиков :
> >>>>>>>
> >>>>>>>> Alexey.
> >>>>>>>>
> >>>>>>>> PR [1] is waiting for your review.
> >>>>>>>> Please, take a look.
> >>>>>>>>
Re: Internal classes are exposed in public API
> You want say didn't discuss? Yes. > Secondly, yes it took a lot of time due to a lot of changes. Is it problem? No, of course. My point - that your contribution that took a long time, already, can’t be an argument to postpone creation of the API in the current release. > 23 янв. 2020 г., в 18:11, Andrey Gura написал(а): > >> We don’t discuss your changes and your proposals for the Metric API. > > You want say didn't discuss? Actually we did it [1] but you told ok > let's see code :) > >> So I don’t think we can make the existence of some PR is an argument to >> introduce(or not introduce) this facade. > > You definitely right in case of competition development. But I think > that collaborative development is more effective. Isn't it? > >> Moreover, As far as I know, you developing changes for the Metric API for 5 >> months without public discussion, for now. Let's start it. > > Firsty, with discussion. See above. > Secondly, yes it took a lot of time due to a lot of changes. Is it problem? > >> Let’s do the following: >> 1. Review `IgniteMetric` facade and introduce it to the users as an >> experimental API. > > It just doesn't make sense. Just commit for commit. > >> 2. Discuss your proposal to the Metric API in the separate thread you are >> referencing. > > You a re welcome to thread [1] again. But please, take into account. > because discussion is postponed by you it's late enough to discuss > proposed solution. But of course I'll answer on your questions and > will be glade to your comments and suggestions. > >> I think we should start a discussion from the simplified explanation of: >> 1. API intentions - What we want to gain with it? What is our focus? >> 2. Simple, minimal example of API main interfaces and desired usages. > > All this already described at [1]. You also can take a look on PR (see > MetricSource implementations, there are complex and simple ones). > > [1] > http://apache-ignite-developers.2346864.n4.nabble.com/IEP-35-Metrics-management-in-Ignite-tp43243.html > > On Thu, Jan 23, 2020 at 5:42 PM Николай Ижиков wrote: >> >> Andrey. >> >>> IGNITE-11927 implementation so your changes are incompatible with my changes >> >> We don’t discuss your changes and your proposals for the Metric API. >> So I don’t think we can make the existence of some PR is an argument to >> introduce(or not introduce) this facade. >> Moreover, As far as I know, you developing changes for the Metric API for 5 >> months without public discussion, for now. Let's start it. >> >> Let’s do the following: >> >> 1. Review `IgniteMetric` facade and introduce it to the users as an >> experimental API. >> >> 2. Discuss your proposal to the Metric API in the separate thread you are >> referencing. >> >> I think we should start a discussion from the simplified explanation of: >> >> 1. API intentions - What we want to gain with it? What is our focus? >> 2. Simple, minimal example of API main interfaces and desired usages. >> >> This will help to the community and me personally better understand your >> idea and share feedback. >> >> Thanks. >> >>> 23 янв. 2020 г., в 17:15, Andrey Gura написал(а): >>> >>> Nikolay, >>> >>> as I wrote early in this thread API significantly changed during >>> IGNITE-11927 implementation so your changes are incompatible with my >>> changes. >>> >>> ReadOnlyMetricRegistry will be removed at all (still exists in a >>> couple of places where it uses). >>> >>> Also I don't want to introduce IgniteMetric facade in this rush. In >>> current implementation this interface just provides access to the >>> ReadOnlyMetricManager instance (which will be removed) but it is not >>> enough. >>> >>> I agree with Denis. We should mark current API as experimental and >>> continue IEP-35 development. See my process proposal [1] described >>> early this thread. We can release Apache Ignite 2.8.1 specially for >>> this changes. >>> Public APIs require deeper thinking in order to provide comprehensive, >>> consistent and convenient way of metrics management for end users. >>> >>> Let's add IgniteExperimental, release 2.8 and finish metrics related >>> issues after it. >>> >>> [1] >>> http://apache-ignite-developers.2346864.n4.nabble.com/Internal-classes-are-exposed-in-public-API-tp45146p45185.html >>> >>> >>> On Wed, Jan 22, 2020
Re: Internal classes are exposed in public API
> We don’t discuss your changes and your proposals for the Metric API.
You want say didn't discuss? Actually we did it [1] but you told ok
let's see code :)
> So I don’t think we can make the existence of some PR is an argument to
> introduce(or not introduce) this facade.
You definitely right in case of competition development. But I think
that collaborative development is more effective. Isn't it?
> Moreover, As far as I know, you developing changes for the Metric API for 5
> months without public discussion, for now. Let's start it.
Firsty, with discussion. See above.
Secondly, yes it took a lot of time due to a lot of changes. Is it problem?
> Let’s do the following:
> 1. Review `IgniteMetric` facade and introduce it to the users as an
> experimental API.
It just doesn't make sense. Just commit for commit.
> 2. Discuss your proposal to the Metric API in the separate thread you are
> referencing.
You a re welcome to thread [1] again. But please, take into account.
because discussion is postponed by you it's late enough to discuss
proposed solution. But of course I'll answer on your questions and
will be glade to your comments and suggestions.
> I think we should start a discussion from the simplified explanation of:
> 1. API intentions - What we want to gain with it? What is our focus?
> 2. Simple, minimal example of API main interfaces and desired usages.
All this already described at [1]. You also can take a look on PR (see
MetricSource implementations, there are complex and simple ones).
[1]
http://apache-ignite-developers.2346864.n4.nabble.com/IEP-35-Metrics-management-in-Ignite-tp43243.html
On Thu, Jan 23, 2020 at 5:42 PM Николай Ижиков wrote:
>
> Andrey.
>
> > IGNITE-11927 implementation so your changes are incompatible with my changes
>
> We don’t discuss your changes and your proposals for the Metric API.
> So I don’t think we can make the existence of some PR is an argument to
> introduce(or not introduce) this facade.
> Moreover, As far as I know, you developing changes for the Metric API for 5
> months without public discussion, for now. Let's start it.
>
> Let’s do the following:
>
> 1. Review `IgniteMetric` facade and introduce it to the users as an
> experimental API.
>
> 2. Discuss your proposal to the Metric API in the separate thread you are
> referencing.
>
> I think we should start a discussion from the simplified explanation of:
>
> 1. API intentions - What we want to gain with it? What is our focus?
> 2. Simple, minimal example of API main interfaces and desired usages.
>
> This will help to the community and me personally better understand your idea
> and share feedback.
>
> Thanks.
>
> > 23 янв. 2020 г., в 17:15, Andrey Gura написал(а):
> >
> > Nikolay,
> >
> > as I wrote early in this thread API significantly changed during
> > IGNITE-11927 implementation so your changes are incompatible with my
> > changes.
> >
> > ReadOnlyMetricRegistry will be removed at all (still exists in a
> > couple of places where it uses).
> >
> > Also I don't want to introduce IgniteMetric facade in this rush. In
> > current implementation this interface just provides access to the
> > ReadOnlyMetricManager instance (which will be removed) but it is not
> > enough.
> >
> > I agree with Denis. We should mark current API as experimental and
> > continue IEP-35 development. See my process proposal [1] described
> > early this thread. We can release Apache Ignite 2.8.1 specially for
> > this changes.
> > Public APIs require deeper thinking in order to provide comprehensive,
> > consistent and convenient way of metrics management for end users.
> >
> > Let's add IgniteExperimental, release 2.8 and finish metrics related
> > issues after it.
> >
> > [1]
> > http://apache-ignite-developers.2346864.n4.nabble.com/Internal-classes-are-exposed-in-public-API-tp45146p45185.html
> >
> >
> > On Wed, Jan 22, 2020 at 5:21 PM Николай Ижиков wrote:
> >>
> >> Hello, Igniters.
> >>
> >> * IGNITE-12552: Move ReadOnlyMetricRegistry to public API merged to the
> >> master and cherry-picked to the 2.8.
> >> So the main issue with the Metric API solved.
> >>
> >> * I raised the PR [1] to fix second issue with the new Metric API: absence
> >> of the public Java API to get metrics.
> >> This PR introduces the following changes:
> >>
> >> 1. IgniteMetric interface created: it provides Java API to access Ignite
> >> metrics created with the new Metric API.
> >>
> >> ```
> >> public interface IgniteMetric extends Iterable {
> >>@Nullable
Re: Internal classes are exposed in public API
Andrey.
> IGNITE-11927 implementation so your changes are incompatible with my changes
We don’t discuss your changes and your proposals for the Metric API.
So I don’t think we can make the existence of some PR is an argument to
introduce(or not introduce) this facade.
Moreover, As far as I know, you developing changes for the Metric API for 5
months without public discussion, for now. Let's start it.
Let’s do the following:
1. Review `IgniteMetric` facade and introduce it to the users as an
experimental API.
2. Discuss your proposal to the Metric API in the separate thread you are
referencing.
I think we should start a discussion from the simplified explanation of:
1. API intentions - What we want to gain with it? What is our focus?
2. Simple, minimal example of API main interfaces and desired usages.
This will help to the community and me personally better understand your idea
and share feedback.
Thanks.
> 23 янв. 2020 г., в 17:15, Andrey Gura написал(а):
>
> Nikolay,
>
> as I wrote early in this thread API significantly changed during
> IGNITE-11927 implementation so your changes are incompatible with my
> changes.
>
> ReadOnlyMetricRegistry will be removed at all (still exists in a
> couple of places where it uses).
>
> Also I don't want to introduce IgniteMetric facade in this rush. In
> current implementation this interface just provides access to the
> ReadOnlyMetricManager instance (which will be removed) but it is not
> enough.
>
> I agree with Denis. We should mark current API as experimental and
> continue IEP-35 development. See my process proposal [1] described
> early this thread. We can release Apache Ignite 2.8.1 specially for
> this changes.
> Public APIs require deeper thinking in order to provide comprehensive,
> consistent and convenient way of metrics management for end users.
>
> Let's add IgniteExperimental, release 2.8 and finish metrics related
> issues after it.
>
> [1]
> http://apache-ignite-developers.2346864.n4.nabble.com/Internal-classes-are-exposed-in-public-API-tp45146p45185.html
>
>
> On Wed, Jan 22, 2020 at 5:21 PM Николай Ижиков wrote:
>>
>> Hello, Igniters.
>>
>> * IGNITE-12552: Move ReadOnlyMetricRegistry to public API merged to the
>> master and cherry-picked to the 2.8.
>> So the main issue with the Metric API solved.
>>
>> * I raised the PR [1] to fix second issue with the new Metric API: absence
>> of the public Java API to get metrics.
>> This PR introduces the following changes:
>>
>> 1. IgniteMetric interface created: it provides Java API to access Ignite
>> metrics created with the new Metric API.
>>
>> ```
>> public interface IgniteMetric extends Iterable {
>>@Nullable ReadOnlyMetricRegistry registry(String name);
>> }
>> ```
>>
>> 2. All deprecation javadocs regarding metrics now reference to the public
>> IgniteMetric instead of internal GridMetricManager:
>>
>>> @deprecated Use {@link IgniteMetric} instead.
>>
>> 3. Tests refactored to use IgniteMetric instead of GridMetricManager when
>> possible.
>>
>> Please, review.
>>
>> [1] https://github.com/apache/ignite/pull/7283
>>
>>> 21 янв. 2020 г., в 17:51, Николай Ижиков
>>> написал(а):
>>>
>>> Hello, Igniters.
>>>
>>> Alexey approved my PR [1] regarding fixing public API for metric exporters.
>>> I’m waiting for a bot visa and merge this PR.
>>>
>>> As we discussed, the metrics API will be marked with IgniteExperimental
>>> annotation.
>>>
>>> If anyone has any objection to this plan, please provide your feedback.
>>>
>>> [1] https://github.com/apache/ignite/pull/7269
>>>
>>>> 21 янв. 2020 г., в 13:45, Николай Ижиков
>>>> написал(а):
>>>>
>>>> Thanks, for the review Alexey.
>>>>
>>>> I will fix your comment and try to implement Monitoring facade, shortly.
>>>>
>>>>> 21 янв. 2020 г., в 13:32, Alexey Goncharuk
>>>>> написал(а):
>>>>>
>>>>> Nikolay,
>>>>>
>>>>> I left a single comment in the PR about the histogram metric. I think the
>>>>> API looks much cleaner now.
>>>>>
>>>>> I will take care of the @IgniteExperimental annotation.
>>>>>
>>>>> пн, 20 янв. 2020 г. в 20:55, Николай Ижиков :
>>>>>
>>>>>> Alexey.
>>>>>>
>>>>>> PR [1] is waiting for your review.
>>>>>> Please, take a look.
>>
Re: Internal classes are exposed in public API
Nikolay,
as I wrote early in this thread API significantly changed during
IGNITE-11927 implementation so your changes are incompatible with my
changes.
ReadOnlyMetricRegistry will be removed at all (still exists in a
couple of places where it uses).
Also I don't want to introduce IgniteMetric facade in this rush. In
current implementation this interface just provides access to the
ReadOnlyMetricManager instance (which will be removed) but it is not
enough.
I agree with Denis. We should mark current API as experimental and
continue IEP-35 development. See my process proposal [1] described
early this thread. We can release Apache Ignite 2.8.1 specially for
this changes.
Public APIs require deeper thinking in order to provide comprehensive,
consistent and convenient way of metrics management for end users.
Let's add IgniteExperimental, release 2.8 and finish metrics related
issues after it.
[1]
http://apache-ignite-developers.2346864.n4.nabble.com/Internal-classes-are-exposed-in-public-API-tp45146p45185.html
On Wed, Jan 22, 2020 at 5:21 PM Николай Ижиков wrote:
>
> Hello, Igniters.
>
> * IGNITE-12552: Move ReadOnlyMetricRegistry to public API merged to the
> master and cherry-picked to the 2.8.
> So the main issue with the Metric API solved.
>
> * I raised the PR [1] to fix second issue with the new Metric API: absence of
> the public Java API to get metrics.
> This PR introduces the following changes:
>
> 1. IgniteMetric interface created: it provides Java API to access Ignite
> metrics created with the new Metric API.
>
> ```
> public interface IgniteMetric extends Iterable {
> @Nullable ReadOnlyMetricRegistry registry(String name);
> }
> ```
>
> 2. All deprecation javadocs regarding metrics now reference to the public
> IgniteMetric instead of internal GridMetricManager:
>
> > @deprecated Use {@link IgniteMetric} instead.
>
> 3. Tests refactored to use IgniteMetric instead of GridMetricManager when
> possible.
>
> Please, review.
>
> [1] https://github.com/apache/ignite/pull/7283
>
> > 21 янв. 2020 г., в 17:51, Николай Ижиков
> > написал(а):
> >
> > Hello, Igniters.
> >
> > Alexey approved my PR [1] regarding fixing public API for metric exporters.
> > I’m waiting for a bot visa and merge this PR.
> >
> > As we discussed, the metrics API will be marked with IgniteExperimental
> > annotation.
> >
> > If anyone has any objection to this plan, please provide your feedback.
> >
> > [1] https://github.com/apache/ignite/pull/7269
> >
> >> 21 янв. 2020 г., в 13:45, Николай Ижиков
> >> написал(а):
> >>
> >> Thanks, for the review Alexey.
> >>
> >> I will fix your comment and try to implement Monitoring facade, shortly.
> >>
> >>> 21 янв. 2020 г., в 13:32, Alexey Goncharuk
> >>> написал(а):
> >>>
> >>> Nikolay,
> >>>
> >>> I left a single comment in the PR about the histogram metric. I think the
> >>> API looks much cleaner now.
> >>>
> >>> I will take care of the @IgniteExperimental annotation.
> >>>
> >>> пн, 20 янв. 2020 г. в 20:55, Николай Ижиков :
> >>>
> >>>> Alexey.
> >>>>
> >>>> PR [1] is waiting for your review.
> >>>> Please, take a look.
> >>>>
> >>>> I think we should do the following before 2.8 release
> >>>>
> >>>> * Introduce new @IgniteExperimental annotation as discussed.
> >>>> * Mark Monitoring API with it.
> >>>> * merge «[IEP-35] Expose MetricRegistry to the public API» to 2.8
> >>>> * merge «[IEP-35] public Java metric API» to 2.8
> >>>>
> >>>> [1] https://github.com/apache/ignite/pull/7269
> >>>>
> >>>>> 20 янв. 2020 г., в 17:09, Alexey Goncharuk
> >>>> написал(а):
> >>>>>
> >>>>> Nikolay,
> >>>>>
> >>>>> Should we wait for both of the tickets given that we agreed that we are
> >>>>> putting an experimental marker on the new APIs? I'm ok to fix only the
> >>>>> first one and add the experimental marker so that we do not delay 2.8
> >>>>> release.
> >>>>>
> >>>>> пн, 20 янв. 2020 г. в 13:32, Николай Ижиков :
> >>>>>
> >>>>>> Andrey.
> >>>>>>
> >>>>>> Let’s move from the long letters to the code.
> >>>>>> If you want to change API - please, propose this changes.
> >>
Re: Internal classes are exposed in public API
Hello, Igniters.
* IGNITE-12552: Move ReadOnlyMetricRegistry to public API merged to the master
and cherry-picked to the 2.8.
So the main issue with the Metric API solved.
* I raised the PR [1] to fix second issue with the new Metric API: absence of
the public Java API to get metrics.
This PR introduces the following changes:
1. IgniteMetric interface created: it provides Java API to access Ignite
metrics created with the new Metric API.
```
public interface IgniteMetric extends Iterable {
@Nullable ReadOnlyMetricRegistry registry(String name);
}
```
2. All deprecation javadocs regarding metrics now reference to the public
IgniteMetric instead of internal GridMetricManager:
> @deprecated Use {@link IgniteMetric} instead.
3. Tests refactored to use IgniteMetric instead of GridMetricManager when
possible.
Please, review.
[1] https://github.com/apache/ignite/pull/7283
> 21 янв. 2020 г., в 17:51, Николай Ижиков написал(а):
>
> Hello, Igniters.
>
> Alexey approved my PR [1] regarding fixing public API for metric exporters.
> I’m waiting for a bot visa and merge this PR.
>
> As we discussed, the metrics API will be marked with IgniteExperimental
> annotation.
>
> If anyone has any objection to this plan, please provide your feedback.
>
> [1] https://github.com/apache/ignite/pull/7269
>
>> 21 янв. 2020 г., в 13:45, Николай Ижиков написал(а):
>>
>> Thanks, for the review Alexey.
>>
>> I will fix your comment and try to implement Monitoring facade, shortly.
>>
>>> 21 янв. 2020 г., в 13:32, Alexey Goncharuk
>>> написал(а):
>>>
>>> Nikolay,
>>>
>>> I left a single comment in the PR about the histogram metric. I think the
>>> API looks much cleaner now.
>>>
>>> I will take care of the @IgniteExperimental annotation.
>>>
>>> пн, 20 янв. 2020 г. в 20:55, Николай Ижиков :
>>>
Alexey.
PR [1] is waiting for your review.
Please, take a look.
I think we should do the following before 2.8 release
* Introduce new @IgniteExperimental annotation as discussed.
* Mark Monitoring API with it.
* merge «[IEP-35] Expose MetricRegistry to the public API» to 2.8
* merge «[IEP-35] public Java metric API» to 2.8
[1] https://github.com/apache/ignite/pull/7269
> 20 янв. 2020 г., в 17:09, Alexey Goncharuk
написал(а):
>
> Nikolay,
>
> Should we wait for both of the tickets given that we agreed that we are
> putting an experimental marker on the new APIs? I'm ok to fix only the
> first one and add the experimental marker so that we do not delay 2.8
> release.
>
> пн, 20 янв. 2020 г. в 13:32, Николай Ижиков :
>
>> Andrey.
>>
>> Let’s move from the long letters to the code.
>> If you want to change API - please, propose this changes.
>> I think everybody wins if we make our API better.
>>
>> Please, describe proposed changes.
>> It would be great if you have some examples or PR for it.
>>
>> As for this release, I have plans to contribute tickets
>> «[IEP-35] Expose MetricRegistry to the public API» [1] and
>> «[IEP-35] public Java metric API» [2] for it.
>>
>> Any objections on it?
>>
>> [1] https://github.com/apache/ignite/pull/7269
>> [2] https://issues.apache.org/jira/browse/IGNITE-12553
>>
>>
>>> 20 янв. 2020 г., в 13:08, Andrey Gura написал(а):
>>>
>>> It solves problem.
>>>
>>> On Mon, Jan 20, 2020 at 12:09 PM Alexey Goncharuk
>>> wrote:
After giving it some thought, I agree with Denis - there is nothing
>> wrong
with exposing the new APIs, we just need to make it clear that we are
>> still
going to change it.
Should we Introduce something like @IgniteExperimental annotation (I
believe it has been discussed a dozen of times on the dev-list)?
пт, 17 янв. 2020 г. в 23:28, Nikolay Izhikov :
> +1 to mark feature or whole release as EA.
>
> пт, 17 янв. 2020 г., 23:00 Denis Magda :
>
>> Folks, if you don't mind I'll share some thoughts/suggestions as an
>> observer who was not involved in the feature development.
>>
>> It's absolutely 'ok' to deprecate an API that is replaced with a
much
>> better version. However, we should do this only when the new APIs
are
>> production-ready. If there are still many limitations or open items
>> then
>> don't deprecate anything that exists and release the new APIs
>> labeling as
>> early access. What if release the improvements labeling as EA
instead
>> of
>> hiding them completely?
>>
>> I would also encourage us to put aside emotions, don't blame or
point
>> fingers. This IEP is a great initiative and you both have already
>> done a
>> tremendous job by
Re: Internal classes are exposed in public API
Hello, Igniters. Alexey approved my PR [1] regarding fixing public API for metric exporters. I’m waiting for a bot visa and merge this PR. As we discussed, the metrics API will be marked with IgniteExperimental annotation. If anyone has any objection to this plan, please provide your feedback. [1] https://github.com/apache/ignite/pull/7269 > 21 янв. 2020 г., в 13:45, Николай Ижиков написал(а): > > Thanks, for the review Alexey. > > I will fix your comment and try to implement Monitoring facade, shortly. > >> 21 янв. 2020 г., в 13:32, Alexey Goncharuk >> написал(а): >> >> Nikolay, >> >> I left a single comment in the PR about the histogram metric. I think the >> API looks much cleaner now. >> >> I will take care of the @IgniteExperimental annotation. >> >> пн, 20 янв. 2020 г. в 20:55, Николай Ижиков : >> >>> Alexey. >>> >>> PR [1] is waiting for your review. >>> Please, take a look. >>> >>> I think we should do the following before 2.8 release >>> >>> * Introduce new @IgniteExperimental annotation as discussed. >>> * Mark Monitoring API with it. >>> * merge «[IEP-35] Expose MetricRegistry to the public API» to 2.8 >>> * merge «[IEP-35] public Java metric API» to 2.8 >>> >>> [1] https://github.com/apache/ignite/pull/7269 >>> 20 янв. 2020 г., в 17:09, Alexey Goncharuk >>> написал(а): Nikolay, Should we wait for both of the tickets given that we agreed that we are putting an experimental marker on the new APIs? I'm ok to fix only the first one and add the experimental marker so that we do not delay 2.8 release. пн, 20 янв. 2020 г. в 13:32, Николай Ижиков : > Andrey. > > Let’s move from the long letters to the code. > If you want to change API - please, propose this changes. > I think everybody wins if we make our API better. > > Please, describe proposed changes. > It would be great if you have some examples or PR for it. > > As for this release, I have plans to contribute tickets > «[IEP-35] Expose MetricRegistry to the public API» [1] and > «[IEP-35] public Java metric API» [2] for it. > > Any objections on it? > > [1] https://github.com/apache/ignite/pull/7269 > [2] https://issues.apache.org/jira/browse/IGNITE-12553 > > >> 20 янв. 2020 г., в 13:08, Andrey Gura написал(а): >> >> It solves problem. >> >> On Mon, Jan 20, 2020 at 12:09 PM Alexey Goncharuk >> wrote: >>> >>> After giving it some thought, I agree with Denis - there is nothing > wrong >>> with exposing the new APIs, we just need to make it clear that we are > still >>> going to change it. >>> >>> Should we Introduce something like @IgniteExperimental annotation (I >>> believe it has been discussed a dozen of times on the dev-list)? >>> >>> пт, 17 янв. 2020 г. в 23:28, Nikolay Izhikov : >>> +1 to mark feature or whole release as EA. пт, 17 янв. 2020 г., 23:00 Denis Magda : > Folks, if you don't mind I'll share some thoughts/suggestions as an > observer who was not involved in the feature development. > > It's absolutely 'ok' to deprecate an API that is replaced with a >>> much > better version. However, we should do this only when the new APIs >>> are > production-ready. If there are still many limitations or open items > then > don't deprecate anything that exists and release the new APIs > labeling as > early access. What if release the improvements labeling as EA >>> instead > of > hiding them completely? > > I would also encourage us to put aside emotions, don't blame or >>> point > fingers. This IEP is a great initiative and you both have already > done a > tremendous job by developing, architecting and reviewing changes. > Just be > respectful. Nobody is trying to block the feature from being >>> released. > Everyone would be glad to tap into improvements and start using them > in > prod. But if more time is needed for the GA let's reiterate a bit. > > - > Denis > > > On Fri, Jan 17, 2020 at 12:24 PM Николай Ижиков < >>> nizhi...@apache.org> > wrote: > >>> Also I agree with Alexey about introducing public IgniteMonitoring > facade >> >> This is not an issue with the API. >> Just the new feature that doesn’t affects API. >> Moreover, I create a ticket to fix it, already. >> >>> It's right. But if you add checking of statisticsEnabling property then >> test will fail. It's just not good tests. >> >> My changes doesn’t affect any `staticticsEnabled` property. >> I don’ understand your point here. >> >>> Redundant
Re: Internal classes are exposed in public API
Thanks, for the review Alexey. I will fix your comment and try to implement Monitoring facade, shortly. > 21 янв. 2020 г., в 13:32, Alexey Goncharuk > написал(а): > > Nikolay, > > I left a single comment in the PR about the histogram metric. I think the > API looks much cleaner now. > > I will take care of the @IgniteExperimental annotation. > > пн, 20 янв. 2020 г. в 20:55, Николай Ижиков : > >> Alexey. >> >> PR [1] is waiting for your review. >> Please, take a look. >> >> I think we should do the following before 2.8 release >> >> * Introduce new @IgniteExperimental annotation as discussed. >> * Mark Monitoring API with it. >> * merge «[IEP-35] Expose MetricRegistry to the public API» to 2.8 >> * merge «[IEP-35] public Java metric API» to 2.8 >> >> [1] https://github.com/apache/ignite/pull/7269 >> >>> 20 янв. 2020 г., в 17:09, Alexey Goncharuk >> написал(а): >>> >>> Nikolay, >>> >>> Should we wait for both of the tickets given that we agreed that we are >>> putting an experimental marker on the new APIs? I'm ok to fix only the >>> first one and add the experimental marker so that we do not delay 2.8 >>> release. >>> >>> пн, 20 янв. 2020 г. в 13:32, Николай Ижиков : >>> Andrey. Let’s move from the long letters to the code. If you want to change API - please, propose this changes. I think everybody wins if we make our API better. Please, describe proposed changes. It would be great if you have some examples or PR for it. As for this release, I have plans to contribute tickets «[IEP-35] Expose MetricRegistry to the public API» [1] and «[IEP-35] public Java metric API» [2] for it. Any objections on it? [1] https://github.com/apache/ignite/pull/7269 [2] https://issues.apache.org/jira/browse/IGNITE-12553 > 20 янв. 2020 г., в 13:08, Andrey Gura написал(а): > > It solves problem. > > On Mon, Jan 20, 2020 at 12:09 PM Alexey Goncharuk > wrote: >> >> After giving it some thought, I agree with Denis - there is nothing wrong >> with exposing the new APIs, we just need to make it clear that we are still >> going to change it. >> >> Should we Introduce something like @IgniteExperimental annotation (I >> believe it has been discussed a dozen of times on the dev-list)? >> >> пт, 17 янв. 2020 г. в 23:28, Nikolay Izhikov : >> >>> +1 to mark feature or whole release as EA. >>> >>> пт, 17 янв. 2020 г., 23:00 Denis Magda : >>> Folks, if you don't mind I'll share some thoughts/suggestions as an observer who was not involved in the feature development. It's absolutely 'ok' to deprecate an API that is replaced with a >> much better version. However, we should do this only when the new APIs >> are production-ready. If there are still many limitations or open items then don't deprecate anything that exists and release the new APIs labeling as early access. What if release the improvements labeling as EA >> instead of hiding them completely? I would also encourage us to put aside emotions, don't blame or >> point fingers. This IEP is a great initiative and you both have already done a tremendous job by developing, architecting and reviewing changes. Just be respectful. Nobody is trying to block the feature from being >> released. Everyone would be glad to tap into improvements and start using them in prod. But if more time is needed for the GA let's reiterate a bit. - Denis On Fri, Jan 17, 2020 at 12:24 PM Николай Ижиков < >> nizhi...@apache.org> wrote: >> Also I agree with Alexey about introducing public IgniteMonitoring facade > > This is not an issue with the API. > Just the new feature that doesn’t affects API. > Moreover, I create a ticket to fix it, already. > >> It's right. But if you add checking of statisticsEnabling property >>> then > test will fail. It's just not good tests. > > My changes doesn’t affect any `staticticsEnabled` property. > I don’ understand your point here. > >> Redundant ReadOnlyMetricRegistry. > > It’s not redundant. > It required for exporters and provide read only access to >>> MetricRegistry > existing in the node. > > >> MetricExporterSpi that requires ReadOnlyMetricRegistry. > >> Absence of newly created metrics in old MBeans that forces user >> use >> exporter SPI while his code base uses old MBeans. > > Why this is issue? > >> Inconsistent MetricRegistry API. > > It’s
Re: Internal classes are exposed in public API
Nikolay, I left a single comment in the PR about the histogram metric. I think the API looks much cleaner now. I will take care of the @IgniteExperimental annotation. пн, 20 янв. 2020 г. в 20:55, Николай Ижиков : > Alexey. > > PR [1] is waiting for your review. > Please, take a look. > > I think we should do the following before 2.8 release > > * Introduce new @IgniteExperimental annotation as discussed. > * Mark Monitoring API with it. > * merge «[IEP-35] Expose MetricRegistry to the public API» to 2.8 > * merge «[IEP-35] public Java metric API» to 2.8 > > [1] https://github.com/apache/ignite/pull/7269 > > > 20 янв. 2020 г., в 17:09, Alexey Goncharuk > написал(а): > > > > Nikolay, > > > > Should we wait for both of the tickets given that we agreed that we are > > putting an experimental marker on the new APIs? I'm ok to fix only the > > first one and add the experimental marker so that we do not delay 2.8 > > release. > > > > пн, 20 янв. 2020 г. в 13:32, Николай Ижиков : > > > >> Andrey. > >> > >> Let’s move from the long letters to the code. > >> If you want to change API - please, propose this changes. > >> I think everybody wins if we make our API better. > >> > >> Please, describe proposed changes. > >> It would be great if you have some examples or PR for it. > >> > >> As for this release, I have plans to contribute tickets > >> «[IEP-35] Expose MetricRegistry to the public API» [1] and > >> «[IEP-35] public Java metric API» [2] for it. > >> > >> Any objections on it? > >> > >> [1] https://github.com/apache/ignite/pull/7269 > >> [2] https://issues.apache.org/jira/browse/IGNITE-12553 > >> > >> > >>> 20 янв. 2020 г., в 13:08, Andrey Gura написал(а): > >>> > >>> It solves problem. > >>> > >>> On Mon, Jan 20, 2020 at 12:09 PM Alexey Goncharuk > >>> wrote: > > After giving it some thought, I agree with Denis - there is nothing > >> wrong > with exposing the new APIs, we just need to make it clear that we are > >> still > going to change it. > > Should we Introduce something like @IgniteExperimental annotation (I > believe it has been discussed a dozen of times on the dev-list)? > > пт, 17 янв. 2020 г. в 23:28, Nikolay Izhikov : > > > +1 to mark feature or whole release as EA. > > > > пт, 17 янв. 2020 г., 23:00 Denis Magda : > > > >> Folks, if you don't mind I'll share some thoughts/suggestions as an > >> observer who was not involved in the feature development. > >> > >> It's absolutely 'ok' to deprecate an API that is replaced with a > much > >> better version. However, we should do this only when the new APIs > are > >> production-ready. If there are still many limitations or open items > >> then > >> don't deprecate anything that exists and release the new APIs > >> labeling as > >> early access. What if release the improvements labeling as EA > instead > >> of > >> hiding them completely? > >> > >> I would also encourage us to put aside emotions, don't blame or > point > >> fingers. This IEP is a great initiative and you both have already > >> done a > >> tremendous job by developing, architecting and reviewing changes. > >> Just be > >> respectful. Nobody is trying to block the feature from being > released. > >> Everyone would be glad to tap into improvements and start using them > >> in > >> prod. But if more time is needed for the GA let's reiterate a bit. > >> > >> - > >> Denis > >> > >> > >> On Fri, Jan 17, 2020 at 12:24 PM Николай Ижиков < > nizhi...@apache.org> > >> wrote: > >> > Also I agree with Alexey about introducing public IgniteMonitoring > >> facade > >>> > >>> This is not an issue with the API. > >>> Just the new feature that doesn’t affects API. > >>> Moreover, I create a ticket to fix it, already. > >>> > It's right. But if you add checking of statisticsEnabling property > > then > >>> test will fail. It's just not good tests. > >>> > >>> My changes doesn’t affect any `staticticsEnabled` property. > >>> I don’ understand your point here. > >>> > Redundant ReadOnlyMetricRegistry. > >>> > >>> It’s not redundant. > >>> It required for exporters and provide read only access to > > MetricRegistry > >>> existing in the node. > >>> > >>> > MetricExporterSpi that requires ReadOnlyMetricRegistry. > >>> > Absence of newly created metrics in old MBeans that forces user > use > exporter SPI while his code base uses old MBeans. > >>> > >>> Why this is issue? > >>> > Inconsistent MetricRegistry API. > >>> > >>> It’s consistent. > >>> > Metrics lookups from map instead of using direct reference > (performance problem). > >>> > >>> 1. We(You and I) did this choice together to simplify creation of > the > > new > >>> metrics. > >>>
Re: Internal classes are exposed in public API
Alexey. PR [1] is waiting for your review. Please, take a look. I think we should do the following before 2.8 release * Introduce new @IgniteExperimental annotation as discussed. * Mark Monitoring API with it. * merge «[IEP-35] Expose MetricRegistry to the public API» to 2.8 * merge «[IEP-35] public Java metric API» to 2.8 [1] https://github.com/apache/ignite/pull/7269 > 20 янв. 2020 г., в 17:09, Alexey Goncharuk > написал(а): > > Nikolay, > > Should we wait for both of the tickets given that we agreed that we are > putting an experimental marker on the new APIs? I'm ok to fix only the > first one and add the experimental marker so that we do not delay 2.8 > release. > > пн, 20 янв. 2020 г. в 13:32, Николай Ижиков : > >> Andrey. >> >> Let’s move from the long letters to the code. >> If you want to change API - please, propose this changes. >> I think everybody wins if we make our API better. >> >> Please, describe proposed changes. >> It would be great if you have some examples or PR for it. >> >> As for this release, I have plans to contribute tickets >> «[IEP-35] Expose MetricRegistry to the public API» [1] and >> «[IEP-35] public Java metric API» [2] for it. >> >> Any objections on it? >> >> [1] https://github.com/apache/ignite/pull/7269 >> [2] https://issues.apache.org/jira/browse/IGNITE-12553 >> >> >>> 20 янв. 2020 г., в 13:08, Andrey Gura написал(а): >>> >>> It solves problem. >>> >>> On Mon, Jan 20, 2020 at 12:09 PM Alexey Goncharuk >>> wrote: After giving it some thought, I agree with Denis - there is nothing >> wrong with exposing the new APIs, we just need to make it clear that we are >> still going to change it. Should we Introduce something like @IgniteExperimental annotation (I believe it has been discussed a dozen of times on the dev-list)? пт, 17 янв. 2020 г. в 23:28, Nikolay Izhikov : > +1 to mark feature or whole release as EA. > > пт, 17 янв. 2020 г., 23:00 Denis Magda : > >> Folks, if you don't mind I'll share some thoughts/suggestions as an >> observer who was not involved in the feature development. >> >> It's absolutely 'ok' to deprecate an API that is replaced with a much >> better version. However, we should do this only when the new APIs are >> production-ready. If there are still many limitations or open items >> then >> don't deprecate anything that exists and release the new APIs >> labeling as >> early access. What if release the improvements labeling as EA instead >> of >> hiding them completely? >> >> I would also encourage us to put aside emotions, don't blame or point >> fingers. This IEP is a great initiative and you both have already >> done a >> tremendous job by developing, architecting and reviewing changes. >> Just be >> respectful. Nobody is trying to block the feature from being released. >> Everyone would be glad to tap into improvements and start using them >> in >> prod. But if more time is needed for the GA let's reiterate a bit. >> >> - >> Denis >> >> >> On Fri, Jan 17, 2020 at 12:24 PM Николай Ижиков >> wrote: >> Also I agree with Alexey about introducing public IgniteMonitoring >> facade >>> >>> This is not an issue with the API. >>> Just the new feature that doesn’t affects API. >>> Moreover, I create a ticket to fix it, already. >>> It's right. But if you add checking of statisticsEnabling property > then >>> test will fail. It's just not good tests. >>> >>> My changes doesn’t affect any `staticticsEnabled` property. >>> I don’ understand your point here. >>> Redundant ReadOnlyMetricRegistry. >>> >>> It’s not redundant. >>> It required for exporters and provide read only access to > MetricRegistry >>> existing in the node. >>> >>> MetricExporterSpi that requires ReadOnlyMetricRegistry. >>> Absence of newly created metrics in old MBeans that forces user use exporter SPI while his code base uses old MBeans. >>> >>> Why this is issue? >>> Inconsistent MetricRegistry API. >>> >>> It’s consistent. >>> Metrics lookups from map instead of using direct reference (performance problem). >>> >>> 1. We(You and I) did this choice together to simplify creation of the > new >>> metrics. >>> 2. This is not public API issue. >>> >>> Ignoring of statisticsEnabled flag. >>> >>> I don’t ignore this flag. >>> It just doesn’t exists in new framework(because of scope). >>> I don’t think it’s an issue. >>> JmxExporterSpi creates beans that doesn't satisfy best MBeans >> practices. >>> >>> Please, clarify your statement. >>> What is best MBeans practices you are talking about? >>> Not finished
Re: Internal classes are exposed in public API
> Let’s move from the long letters to the code. Let's thinking before coding. Development is not only coding. Why such a rush? Measure thrice and cut once. "Long letters" is way of discussion before writing code. And it's ok. On Mon, Jan 20, 2020 at 1:32 PM Николай Ижиков wrote: > > Andrey. > > Let’s move from the long letters to the code. > If you want to change API - please, propose this changes. > I think everybody wins if we make our API better. > > Please, describe proposed changes. > It would be great if you have some examples or PR for it. > > As for this release, I have plans to contribute tickets > «[IEP-35] Expose MetricRegistry to the public API» [1] and > «[IEP-35] public Java metric API» [2] for it. > > Any objections on it? > > [1] https://github.com/apache/ignite/pull/7269 > [2] https://issues.apache.org/jira/browse/IGNITE-12553 > > > > 20 янв. 2020 г., в 13:08, Andrey Gura написал(а): > > > > It solves problem. > > > > On Mon, Jan 20, 2020 at 12:09 PM Alexey Goncharuk > > wrote: > >> > >> After giving it some thought, I agree with Denis - there is nothing wrong > >> with exposing the new APIs, we just need to make it clear that we are still > >> going to change it. > >> > >> Should we Introduce something like @IgniteExperimental annotation (I > >> believe it has been discussed a dozen of times on the dev-list)? > >> > >> пт, 17 янв. 2020 г. в 23:28, Nikolay Izhikov : > >> > >>> +1 to mark feature or whole release as EA. > >>> > >>> пт, 17 янв. 2020 г., 23:00 Denis Magda : > >>> > Folks, if you don't mind I'll share some thoughts/suggestions as an > observer who was not involved in the feature development. > > It's absolutely 'ok' to deprecate an API that is replaced with a much > better version. However, we should do this only when the new APIs are > production-ready. If there are still many limitations or open items then > don't deprecate anything that exists and release the new APIs labeling as > early access. What if release the improvements labeling as EA instead of > hiding them completely? > > I would also encourage us to put aside emotions, don't blame or point > fingers. This IEP is a great initiative and you both have already done a > tremendous job by developing, architecting and reviewing changes. Just be > respectful. Nobody is trying to block the feature from being released. > Everyone would be glad to tap into improvements and start using them in > prod. But if more time is needed for the GA let's reiterate a bit. > > - > Denis > > > On Fri, Jan 17, 2020 at 12:24 PM Николай Ижиков > wrote: > > >> Also I agree with Alexey about introducing public IgniteMonitoring > facade > > > > This is not an issue with the API. > > Just the new feature that doesn’t affects API. > > Moreover, I create a ticket to fix it, already. > > > >> It's right. But if you add checking of statisticsEnabling property > >>> then > > test will fail. It's just not good tests. > > > > My changes doesn’t affect any `staticticsEnabled` property. > > I don’ understand your point here. > > > >> Redundant ReadOnlyMetricRegistry. > > > > It’s not redundant. > > It required for exporters and provide read only access to > >>> MetricRegistry > > existing in the node. > > > > > >> MetricExporterSpi that requires ReadOnlyMetricRegistry. > > > >> Absence of newly created metrics in old MBeans that forces user use > >> exporter SPI while his code base uses old MBeans. > > > > Why this is issue? > > > >> Inconsistent MetricRegistry API. > > > > It’s consistent. > > > >> Metrics lookups from map instead of using direct reference > >> (performance problem). > > > > 1. We(You and I) did this choice together to simplify creation of the > >>> new > > metrics. > > 2. This is not public API issue. > > > > > >> Ignoring of statisticsEnabled flag. > > > > I don’t ignore this flag. > > It just doesn’t exists in new framework(because of scope). > > I don’t think it’s an issue. > > > >> JmxExporterSpi creates beans that doesn't satisfy best MBeans > practices. > > > > Please, clarify your statement. > > What is best MBeans practices you are talking about? > > > >> Not finished IGNITE-11927 > > > > > > How this can be API issue? > > > > > >> 17 янв. 2020 г., в 20:52, Andrey Gura написал(а): > >> > All issues Alexey mentioned in starting letter are fixed with my PR > > [1]. > I don’t think other issues you mentioned are blocker for the > >>> release. > >> > >> As I mentioned already IGNITE-11927 is part of IEP-35 and > >> implementation seriously affects API's. Also I agree with Alexey > >>> about > >> introducing public IgniteMonitoring
Re: Internal classes are exposed in public API
Nikolay, Should we wait for both of the tickets given that we agreed that we are putting an experimental marker on the new APIs? I'm ok to fix only the first one and add the experimental marker so that we do not delay 2.8 release. пн, 20 янв. 2020 г. в 13:32, Николай Ижиков : > Andrey. > > Let’s move from the long letters to the code. > If you want to change API - please, propose this changes. > I think everybody wins if we make our API better. > > Please, describe proposed changes. > It would be great if you have some examples or PR for it. > > As for this release, I have plans to contribute tickets > «[IEP-35] Expose MetricRegistry to the public API» [1] and > «[IEP-35] public Java metric API» [2] for it. > > Any objections on it? > > [1] https://github.com/apache/ignite/pull/7269 > [2] https://issues.apache.org/jira/browse/IGNITE-12553 > > > > 20 янв. 2020 г., в 13:08, Andrey Gura написал(а): > > > > It solves problem. > > > > On Mon, Jan 20, 2020 at 12:09 PM Alexey Goncharuk > > wrote: > >> > >> After giving it some thought, I agree with Denis - there is nothing > wrong > >> with exposing the new APIs, we just need to make it clear that we are > still > >> going to change it. > >> > >> Should we Introduce something like @IgniteExperimental annotation (I > >> believe it has been discussed a dozen of times on the dev-list)? > >> > >> пт, 17 янв. 2020 г. в 23:28, Nikolay Izhikov : > >> > >>> +1 to mark feature or whole release as EA. > >>> > >>> пт, 17 янв. 2020 г., 23:00 Denis Magda : > >>> > Folks, if you don't mind I'll share some thoughts/suggestions as an > observer who was not involved in the feature development. > > It's absolutely 'ok' to deprecate an API that is replaced with a much > better version. However, we should do this only when the new APIs are > production-ready. If there are still many limitations or open items > then > don't deprecate anything that exists and release the new APIs > labeling as > early access. What if release the improvements labeling as EA instead > of > hiding them completely? > > I would also encourage us to put aside emotions, don't blame or point > fingers. This IEP is a great initiative and you both have already > done a > tremendous job by developing, architecting and reviewing changes. > Just be > respectful. Nobody is trying to block the feature from being released. > Everyone would be glad to tap into improvements and start using them > in > prod. But if more time is needed for the GA let's reiterate a bit. > > - > Denis > > > On Fri, Jan 17, 2020 at 12:24 PM Николай Ижиков > wrote: > > >> Also I agree with Alexey about introducing public IgniteMonitoring > facade > > > > This is not an issue with the API. > > Just the new feature that doesn’t affects API. > > Moreover, I create a ticket to fix it, already. > > > >> It's right. But if you add checking of statisticsEnabling property > >>> then > > test will fail. It's just not good tests. > > > > My changes doesn’t affect any `staticticsEnabled` property. > > I don’ understand your point here. > > > >> Redundant ReadOnlyMetricRegistry. > > > > It’s not redundant. > > It required for exporters and provide read only access to > >>> MetricRegistry > > existing in the node. > > > > > >> MetricExporterSpi that requires ReadOnlyMetricRegistry. > > > >> Absence of newly created metrics in old MBeans that forces user use > >> exporter SPI while his code base uses old MBeans. > > > > Why this is issue? > > > >> Inconsistent MetricRegistry API. > > > > It’s consistent. > > > >> Metrics lookups from map instead of using direct reference > >> (performance problem). > > > > 1. We(You and I) did this choice together to simplify creation of the > >>> new > > metrics. > > 2. This is not public API issue. > > > > > >> Ignoring of statisticsEnabled flag. > > > > I don’t ignore this flag. > > It just doesn’t exists in new framework(because of scope). > > I don’t think it’s an issue. > > > >> JmxExporterSpi creates beans that doesn't satisfy best MBeans > practices. > > > > Please, clarify your statement. > > What is best MBeans practices you are talking about? > > > >> Not finished IGNITE-11927 > > > > > > How this can be API issue? > > > > > >> 17 янв. 2020 г., в 20:52, Andrey Gura > написал(а): > >> > All issues Alexey mentioned in starting letter are fixed with my > PR > > [1]. > I don’t think other issues you mentioned are blocker for the > >>> release. > >> > >> As I mentioned already IGNITE-11927 is part of IEP-35 and > >> implementation seriously affects API's. Also I agree with Alexey > >>> about > >> introducing public
Re: Internal classes are exposed in public API
Andrey. Let’s move from the long letters to the code. If you want to change API - please, propose this changes. I think everybody wins if we make our API better. Please, describe proposed changes. It would be great if you have some examples or PR for it. As for this release, I have plans to contribute tickets «[IEP-35] Expose MetricRegistry to the public API» [1] and «[IEP-35] public Java metric API» [2] for it. Any objections on it? [1] https://github.com/apache/ignite/pull/7269 [2] https://issues.apache.org/jira/browse/IGNITE-12553 > 20 янв. 2020 г., в 13:08, Andrey Gura написал(а): > > It solves problem. > > On Mon, Jan 20, 2020 at 12:09 PM Alexey Goncharuk > wrote: >> >> After giving it some thought, I agree with Denis - there is nothing wrong >> with exposing the new APIs, we just need to make it clear that we are still >> going to change it. >> >> Should we Introduce something like @IgniteExperimental annotation (I >> believe it has been discussed a dozen of times on the dev-list)? >> >> пт, 17 янв. 2020 г. в 23:28, Nikolay Izhikov : >> >>> +1 to mark feature or whole release as EA. >>> >>> пт, 17 янв. 2020 г., 23:00 Denis Magda : >>> Folks, if you don't mind I'll share some thoughts/suggestions as an observer who was not involved in the feature development. It's absolutely 'ok' to deprecate an API that is replaced with a much better version. However, we should do this only when the new APIs are production-ready. If there are still many limitations or open items then don't deprecate anything that exists and release the new APIs labeling as early access. What if release the improvements labeling as EA instead of hiding them completely? I would also encourage us to put aside emotions, don't blame or point fingers. This IEP is a great initiative and you both have already done a tremendous job by developing, architecting and reviewing changes. Just be respectful. Nobody is trying to block the feature from being released. Everyone would be glad to tap into improvements and start using them in prod. But if more time is needed for the GA let's reiterate a bit. - Denis On Fri, Jan 17, 2020 at 12:24 PM Николай Ижиков wrote: >> Also I agree with Alexey about introducing public IgniteMonitoring facade > > This is not an issue with the API. > Just the new feature that doesn’t affects API. > Moreover, I create a ticket to fix it, already. > >> It's right. But if you add checking of statisticsEnabling property >>> then > test will fail. It's just not good tests. > > My changes doesn’t affect any `staticticsEnabled` property. > I don’ understand your point here. > >> Redundant ReadOnlyMetricRegistry. > > It’s not redundant. > It required for exporters and provide read only access to >>> MetricRegistry > existing in the node. > > >> MetricExporterSpi that requires ReadOnlyMetricRegistry. > >> Absence of newly created metrics in old MBeans that forces user use >> exporter SPI while his code base uses old MBeans. > > Why this is issue? > >> Inconsistent MetricRegistry API. > > It’s consistent. > >> Metrics lookups from map instead of using direct reference >> (performance problem). > > 1. We(You and I) did this choice together to simplify creation of the >>> new > metrics. > 2. This is not public API issue. > > >> Ignoring of statisticsEnabled flag. > > I don’t ignore this flag. > It just doesn’t exists in new framework(because of scope). > I don’t think it’s an issue. > >> JmxExporterSpi creates beans that doesn't satisfy best MBeans practices. > > Please, clarify your statement. > What is best MBeans practices you are talking about? > >> Not finished IGNITE-11927 > > > How this can be API issue? > > >> 17 янв. 2020 г., в 20:52, Andrey Gura написал(а): >> All issues Alexey mentioned in starting letter are fixed with my PR > [1]. I don’t think other issues you mentioned are blocker for the >>> release. >> >> As I mentioned already IGNITE-11927 is part of IEP-35 and >> implementation seriously affects API's. Also I agree with Alexey >>> about >> introducing public IgniteMonitoring facade. So thiss PR doesn't fix >> all issues. >> I talk about ignored existing functionality. >>> There is no existing tests that was broken by this contribution. >> >> It's right. But if you add checking of statisticsEnabling property >> then test will fail. It's just not good tests. >> >>> If you know the issues with it, feel free to create a ticket I will fix > it ASAP. >> >> I already fix it all in IGNITE-11927 >> 1.
Re: Internal classes are exposed in public API
It solves problem. On Mon, Jan 20, 2020 at 12:09 PM Alexey Goncharuk wrote: > > After giving it some thought, I agree with Denis - there is nothing wrong > with exposing the new APIs, we just need to make it clear that we are still > going to change it. > > Should we Introduce something like @IgniteExperimental annotation (I > believe it has been discussed a dozen of times on the dev-list)? > > пт, 17 янв. 2020 г. в 23:28, Nikolay Izhikov : > > > +1 to mark feature or whole release as EA. > > > > пт, 17 янв. 2020 г., 23:00 Denis Magda : > > > > > Folks, if you don't mind I'll share some thoughts/suggestions as an > > > observer who was not involved in the feature development. > > > > > > It's absolutely 'ok' to deprecate an API that is replaced with a much > > > better version. However, we should do this only when the new APIs are > > > production-ready. If there are still many limitations or open items then > > > don't deprecate anything that exists and release the new APIs labeling as > > > early access. What if release the improvements labeling as EA instead of > > > hiding them completely? > > > > > > I would also encourage us to put aside emotions, don't blame or point > > > fingers. This IEP is a great initiative and you both have already done a > > > tremendous job by developing, architecting and reviewing changes. Just be > > > respectful. Nobody is trying to block the feature from being released. > > > Everyone would be glad to tap into improvements and start using them in > > > prod. But if more time is needed for the GA let's reiterate a bit. > > > > > > - > > > Denis > > > > > > > > > On Fri, Jan 17, 2020 at 12:24 PM Николай Ижиков > > > wrote: > > > > > > > > Also I agree with Alexey about introducing public IgniteMonitoring > > > facade > > > > > > > > This is not an issue with the API. > > > > Just the new feature that doesn’t affects API. > > > > Moreover, I create a ticket to fix it, already. > > > > > > > > > It's right. But if you add checking of statisticsEnabling property > > then > > > > test will fail. It's just not good tests. > > > > > > > > My changes doesn’t affect any `staticticsEnabled` property. > > > > I don’ understand your point here. > > > > > > > > > Redundant ReadOnlyMetricRegistry. > > > > > > > > It’s not redundant. > > > > It required for exporters and provide read only access to > > MetricRegistry > > > > existing in the node. > > > > > > > > > > > > > MetricExporterSpi that requires ReadOnlyMetricRegistry. > > > > > > > > > Absence of newly created metrics in old MBeans that forces user use > > > > > exporter SPI while his code base uses old MBeans. > > > > > > > > Why this is issue? > > > > > > > > > Inconsistent MetricRegistry API. > > > > > > > > It’s consistent. > > > > > > > > > Metrics lookups from map instead of using direct reference > > > > > (performance problem). > > > > > > > > 1. We(You and I) did this choice together to simplify creation of the > > new > > > > metrics. > > > > 2. This is not public API issue. > > > > > > > > > > > > > Ignoring of statisticsEnabled flag. > > > > > > > > I don’t ignore this flag. > > > > It just doesn’t exists in new framework(because of scope). > > > > I don’t think it’s an issue. > > > > > > > > > JmxExporterSpi creates beans that doesn't satisfy best MBeans > > > practices. > > > > > > > > Please, clarify your statement. > > > > What is best MBeans practices you are talking about? > > > > > > > > > Not finished IGNITE-11927 > > > > > > > > > > > > How this can be API issue? > > > > > > > > > > > > > 17 янв. 2020 г., в 20:52, Andrey Gura написал(а): > > > > > > > > > >>> All issues Alexey mentioned in starting letter are fixed with my PR > > > > [1]. > > > > >>> I don’t think other issues you mentioned are blocker for the > > release. > > > > > > > > > > As I mentioned already IGNITE-11927 is part of IEP-35 and > > > > > implementation seriously affects API's. Also I agree with Alexey > > about > > > > > introducing public IgniteMonitoring facade. So thiss PR doesn't fix > > > > > all issues. > > > > > > > > > >>> I talk about ignored existing functionality. > > > > >> There is no existing tests that was broken by this contribution. > > > > > > > > > > It's right. But if you add checking of statisticsEnabling property > > > > > then test will fail. It's just not good tests. > > > > > > > > > >> If you know the issues with it, feel free to create a ticket I will > > > fix > > > > it ASAP. > > > > > > > > > > I already fix it all in IGNITE-11927 > > > > > > > > > >>> 1. Moving IEP-35 API's to the internal package. > > > > >> We should move the product forward and shouldn't hide major > > > > contribution from the user just because your second guess «I don’t like > > > the > > > > API I just reviewed and approved». > > > > > > > > > > We should move the product forward with with really finished > > features, > > > > > not pieces of features. > > > > > > > > > >> So I am against this proposal. > > > > > > > > > > It
Re: Internal classes are exposed in public API
> However, we should do this only when the new APIs are production-ready. Denis, the problem is in understanding what does it mean "production ready". Obviously we have different understanding of this term. > What if release the improvements labeling as EA instead of hiding them completely? It's like RC. Usually GA release follows and it must have stable API. > I would also encourage us to put aside emotions, don't blame or point fingers All my finger points are related with API not with person. On Fri, Jan 17, 2020 at 11:00 PM Denis Magda wrote: > > Folks, if you don't mind I'll share some thoughts/suggestions as an > observer who was not involved in the feature development. > > It's absolutely 'ok' to deprecate an API that is replaced with a much > better version. However, we should do this only when the new APIs are > production-ready. If there are still many limitations or open items then > don't deprecate anything that exists and release the new APIs labeling as > early access. What if release the improvements labeling as EA instead of > hiding them completely? > > I would also encourage us to put aside emotions, don't blame or point > fingers. This IEP is a great initiative and you both have already done a > tremendous job by developing, architecting and reviewing changes. Just be > respectful. Nobody is trying to block the feature from being released. > Everyone would be glad to tap into improvements and start using them in > prod. But if more time is needed for the GA let's reiterate a bit. > > - > Denis > > > On Fri, Jan 17, 2020 at 12:24 PM Николай Ижиков wrote: > > > > Also I agree with Alexey about introducing public IgniteMonitoring facade > > > > This is not an issue with the API. > > Just the new feature that doesn’t affects API. > > Moreover, I create a ticket to fix it, already. > > > > > It's right. But if you add checking of statisticsEnabling property then > > test will fail. It's just not good tests. > > > > My changes doesn’t affect any `staticticsEnabled` property. > > I don’ understand your point here. > > > > > Redundant ReadOnlyMetricRegistry. > > > > It’s not redundant. > > It required for exporters and provide read only access to MetricRegistry > > existing in the node. > > > > > > > MetricExporterSpi that requires ReadOnlyMetricRegistry. > > > > > Absence of newly created metrics in old MBeans that forces user use > > > exporter SPI while his code base uses old MBeans. > > > > Why this is issue? > > > > > Inconsistent MetricRegistry API. > > > > It’s consistent. > > > > > Metrics lookups from map instead of using direct reference > > > (performance problem). > > > > 1. We(You and I) did this choice together to simplify creation of the new > > metrics. > > 2. This is not public API issue. > > > > > > > Ignoring of statisticsEnabled flag. > > > > I don’t ignore this flag. > > It just doesn’t exists in new framework(because of scope). > > I don’t think it’s an issue. > > > > > JmxExporterSpi creates beans that doesn't satisfy best MBeans practices. > > > > Please, clarify your statement. > > What is best MBeans practices you are talking about? > > > > > Not finished IGNITE-11927 > > > > > > How this can be API issue? > > > > > > > 17 янв. 2020 г., в 20:52, Andrey Gura написал(а): > > > > > >>> All issues Alexey mentioned in starting letter are fixed with my PR > > [1]. > > >>> I don’t think other issues you mentioned are blocker for the release. > > > > > > As I mentioned already IGNITE-11927 is part of IEP-35 and > > > implementation seriously affects API's. Also I agree with Alexey about > > > introducing public IgniteMonitoring facade. So thiss PR doesn't fix > > > all issues. > > > > > >>> I talk about ignored existing functionality. > > >> There is no existing tests that was broken by this contribution. > > > > > > It's right. But if you add checking of statisticsEnabling property > > > then test will fail. It's just not good tests. > > > > > >> If you know the issues with it, feel free to create a ticket I will fix > > it ASAP. > > > > > > I already fix it all in IGNITE-11927 > > > > > >>> 1. Moving IEP-35 API's to the internal package. > > >> We should move the product forward and shouldn't hide major > > contribution from the user just because your second guess «I don’t like the > > API I just reviewed and approved». > > > > > > We should move the product forward with with really finished features, > > > not pieces of features. > > > > > >> So I am against this proposal. > > > > > > It is not argument. > > > > > >> But, I’m ready to see your proposal for the API change and discuss them. > > > > > > We can prepare it together. But we can't block release. > > > > > >>> Because IGNITE-11927 doesn't solve all problems > > >> What is *ALL* problems? > > > > > > Redundant ReadOnlyMetricRegistry. > > > MetricExporterSpi that requires ReadOnlyMetricRegistry. > > > Absence of newly created metrics in old MBeans that forces user use > > > exporter SPI while his code base
Re: Internal classes are exposed in public API
>> Also I agree with Alexey about introducing public IgniteMonitoring facade > This is not an issue with the API. > Just the new feature that doesn’t affects API. I disagree. It's part of API and it affects user experience. > Moreover, I create a ticket to fix it, already. Creating an issue does not solve the problem. >> Absence of newly created metrics in old MBeans that forces user use >> exporter SPI while his code base uses old MBeans. > Why this is issue? Because it is user experience. As I wrote already user must configure additional SPI in order to gt just a couple of numbers. It is not good idea from point of view. >> Inconsistent MetricRegistry API. > It’s consistent. I described concrete problems with API consistency early in this thread. Jus saying "it's consistent" doesn't make it consistent. >> Metrics lookups from map instead of using direct reference >> (performance problem). > 1. We(You and I) did this choice together to simplify creation of the new > metrics. No. I pointed to this problem. > 2. This is not public API issue. But this is issue. >> Ignoring of statisticsEnabled flag. > I don’t ignore this flag. But flag's value is ignored. > It just doesn’t exists in new framework(because of scope). I don't understand how this scope was formulated. There is some feature. It didn't removed. So it should be taken into account during adding new functionality if it affected. > I don’t think it’s an issue. It's just opinion. Form my point of view it is bug. >> JmxExporterSpi creates beans that doesn't satisfy best MBeans practices. > Please, clarify your statement. > What is best MBeans practices you are talking about? Again. I provided link to the article about it early in this thread. >> Not finished IGNITE-11927 > How this can be API issue? As I wrote early in this thread (may be twice) it changes the API significantly. On Fri, Jan 17, 2020 at 9:24 PM Николай Ижиков wrote: > > > Also I agree with Alexey about introducing public IgniteMonitoring facade > > This is not an issue with the API. > Just the new feature that doesn’t affects API. > Moreover, I create a ticket to fix it, already. > > > It's right. But if you add checking of statisticsEnabling property then > > test will fail. It's just not good tests. > > My changes doesn’t affect any `staticticsEnabled` property. > I don’ understand your point here. > > > Redundant ReadOnlyMetricRegistry. > > It’s not redundant. > It required for exporters and provide read only access to MetricRegistry > existing in the node. > > > > MetricExporterSpi that requires ReadOnlyMetricRegistry. > > > Absence of newly created metrics in old MBeans that forces user use > > exporter SPI while his code base uses old MBeans. > > Why this is issue? > > > Inconsistent MetricRegistry API. > > It’s consistent. > > > Metrics lookups from map instead of using direct reference > > (performance problem). > > 1. We(You and I) did this choice together to simplify creation of the new > metrics. > 2. This is not public API issue. > > > > Ignoring of statisticsEnabled flag. > > I don’t ignore this flag. > It just doesn’t exists in new framework(because of scope). > I don’t think it’s an issue. > > > JmxExporterSpi creates beans that doesn't satisfy best MBeans practices. > > Please, clarify your statement. > What is best MBeans practices you are talking about? > > > Not finished IGNITE-11927 > > > How this can be API issue? > > > > 17 янв. 2020 г., в 20:52, Andrey Gura написал(а): > > > >>> All issues Alexey mentioned in starting letter are fixed with my PR [1]. > >>> I don’t think other issues you mentioned are blocker for the release. > > > > As I mentioned already IGNITE-11927 is part of IEP-35 and > > implementation seriously affects API's. Also I agree with Alexey about > > introducing public IgniteMonitoring facade. So thiss PR doesn't fix > > all issues. > > > >>> I talk about ignored existing functionality. > >> There is no existing tests that was broken by this contribution. > > > > It's right. But if you add checking of statisticsEnabling property > > then test will fail. It's just not good tests. > > > >> If you know the issues with it, feel free to create a ticket I will fix it > >> ASAP. > > > > I already fix it all in IGNITE-11927 > > > >>> 1. Moving IEP-35 API's to the internal package. > >> We should move the product forward and shouldn't hide major contribution > >> from the user just because your second guess «I don’t like the API I just > >> reviewed and approved». > > > > We should move the product forward with with really finished features, > > not pieces of features. > > > >> So I am against this proposal. > > > > It is not argument. > > > >> But, I’m ready to see your proposal for the API change and discuss them. > > > > We can prepare it together. But we can't block release. > > > >>> Because IGNITE-11927 doesn't solve all problems > >> What is *ALL* problems? > > > > Redundant ReadOnlyMetricRegistry. > >
Re: Internal classes are exposed in public API
Hello, I totally agree. It would be nice to have a marker that can be used to indicate that the feature may be changed in future releases and should be used to your own risk. Thanks, S. пн, 20 янв. 2020 г. в 12:16, Anton Vinogradov : > +1 to @IgniteExperimental > > On Mon, Jan 20, 2020 at 12:09 PM Alexey Goncharuk < > alexey.goncha...@gmail.com> wrote: > > > After giving it some thought, I agree with Denis - there is nothing wrong > > with exposing the new APIs, we just need to make it clear that we are > still > > going to change it. > > > > Should we Introduce something like @IgniteExperimental annotation (I > > believe it has been discussed a dozen of times on the dev-list)? > > > > пт, 17 янв. 2020 г. в 23:28, Nikolay Izhikov : > > > > > +1 to mark feature or whole release as EA. > > > > > > пт, 17 янв. 2020 г., 23:00 Denis Magda : > > > > > > > Folks, if you don't mind I'll share some thoughts/suggestions as an > > > > observer who was not involved in the feature development. > > > > > > > > It's absolutely 'ok' to deprecate an API that is replaced with a much > > > > better version. However, we should do this only when the new APIs are > > > > production-ready. If there are still many limitations or open items > > then > > > > don't deprecate anything that exists and release the new APIs > labeling > > as > > > > early access. What if release the improvements labeling as EA instead > > of > > > > hiding them completely? > > > > > > > > I would also encourage us to put aside emotions, don't blame or point > > > > fingers. This IEP is a great initiative and you both have already > done > > a > > > > tremendous job by developing, architecting and reviewing changes. > Just > > be > > > > respectful. Nobody is trying to block the feature from being > released. > > > > Everyone would be glad to tap into improvements and start using them > in > > > > prod. But if more time is needed for the GA let's reiterate a bit. > > > > > > > > - > > > > Denis > > > > > > > > > > > > On Fri, Jan 17, 2020 at 12:24 PM Николай Ижиков > > > > > wrote: > > > > > > > > > > Also I agree with Alexey about introducing public > IgniteMonitoring > > > > facade > > > > > > > > > > This is not an issue with the API. > > > > > Just the new feature that doesn’t affects API. > > > > > Moreover, I create a ticket to fix it, already. > > > > > > > > > > > It's right. But if you add checking of statisticsEnabling > property > > > then > > > > > test will fail. It's just not good tests. > > > > > > > > > > My changes doesn’t affect any `staticticsEnabled` property. > > > > > I don’ understand your point here. > > > > > > > > > > > Redundant ReadOnlyMetricRegistry. > > > > > > > > > > It’s not redundant. > > > > > It required for exporters and provide read only access to > > > MetricRegistry > > > > > existing in the node. > > > > > > > > > > > > > > > > MetricExporterSpi that requires ReadOnlyMetricRegistry. > > > > > > > > > > > Absence of newly created metrics in old MBeans that forces user > use > > > > > > exporter SPI while his code base uses old MBeans. > > > > > > > > > > Why this is issue? > > > > > > > > > > > Inconsistent MetricRegistry API. > > > > > > > > > > It’s consistent. > > > > > > > > > > > Metrics lookups from map instead of using direct reference > > > > > > (performance problem). > > > > > > > > > > 1. We(You and I) did this choice together to simplify creation of > the > > > new > > > > > metrics. > > > > > 2. This is not public API issue. > > > > > > > > > > > > > > > > Ignoring of statisticsEnabled flag. > > > > > > > > > > I don’t ignore this flag. > > > > > It just doesn’t exists in new framework(because of scope). > > > > > I don’t think it’s an issue. > > > > > > > > > > > JmxExporterSpi creates beans that doesn't satisfy best MBeans > > > > practices. > > > > > > > > > > Please, clarify your statement. > > > > > What is best MBeans practices you are talking about? > > > > > > > > > > > Not finished IGNITE-11927 > > > > > > > > > > > > > > > How this can be API issue? > > > > > > > > > > > > > > > > 17 янв. 2020 г., в 20:52, Andrey Gura > > написал(а): > > > > > > > > > > > >>> All issues Alexey mentioned in starting letter are fixed with > my > > PR > > > > > [1]. > > > > > >>> I don’t think other issues you mentioned are blocker for the > > > release. > > > > > > > > > > > > As I mentioned already IGNITE-11927 is part of IEP-35 and > > > > > > implementation seriously affects API's. Also I agree with Alexey > > > about > > > > > > introducing public IgniteMonitoring facade. So thiss PR doesn't > fix > > > > > > all issues. > > > > > > > > > > > >>> I talk about ignored existing functionality. > > > > > >> There is no existing tests that was broken by this contribution. > > > > > > > > > > > > It's right. But if you add checking of statisticsEnabling > property > > > > > > then test will fail. It's just not good tests. > > > > > > > > > > > >> If you know the issues with it, feel free to create a
Re: Internal classes are exposed in public API
+1 to @IgniteExperimental On Mon, Jan 20, 2020 at 12:09 PM Alexey Goncharuk < alexey.goncha...@gmail.com> wrote: > After giving it some thought, I agree with Denis - there is nothing wrong > with exposing the new APIs, we just need to make it clear that we are still > going to change it. > > Should we Introduce something like @IgniteExperimental annotation (I > believe it has been discussed a dozen of times on the dev-list)? > > пт, 17 янв. 2020 г. в 23:28, Nikolay Izhikov : > > > +1 to mark feature or whole release as EA. > > > > пт, 17 янв. 2020 г., 23:00 Denis Magda : > > > > > Folks, if you don't mind I'll share some thoughts/suggestions as an > > > observer who was not involved in the feature development. > > > > > > It's absolutely 'ok' to deprecate an API that is replaced with a much > > > better version. However, we should do this only when the new APIs are > > > production-ready. If there are still many limitations or open items > then > > > don't deprecate anything that exists and release the new APIs labeling > as > > > early access. What if release the improvements labeling as EA instead > of > > > hiding them completely? > > > > > > I would also encourage us to put aside emotions, don't blame or point > > > fingers. This IEP is a great initiative and you both have already done > a > > > tremendous job by developing, architecting and reviewing changes. Just > be > > > respectful. Nobody is trying to block the feature from being released. > > > Everyone would be glad to tap into improvements and start using them in > > > prod. But if more time is needed for the GA let's reiterate a bit. > > > > > > - > > > Denis > > > > > > > > > On Fri, Jan 17, 2020 at 12:24 PM Николай Ижиков > > > wrote: > > > > > > > > Also I agree with Alexey about introducing public IgniteMonitoring > > > facade > > > > > > > > This is not an issue with the API. > > > > Just the new feature that doesn’t affects API. > > > > Moreover, I create a ticket to fix it, already. > > > > > > > > > It's right. But if you add checking of statisticsEnabling property > > then > > > > test will fail. It's just not good tests. > > > > > > > > My changes doesn’t affect any `staticticsEnabled` property. > > > > I don’ understand your point here. > > > > > > > > > Redundant ReadOnlyMetricRegistry. > > > > > > > > It’s not redundant. > > > > It required for exporters and provide read only access to > > MetricRegistry > > > > existing in the node. > > > > > > > > > > > > > MetricExporterSpi that requires ReadOnlyMetricRegistry. > > > > > > > > > Absence of newly created metrics in old MBeans that forces user use > > > > > exporter SPI while his code base uses old MBeans. > > > > > > > > Why this is issue? > > > > > > > > > Inconsistent MetricRegistry API. > > > > > > > > It’s consistent. > > > > > > > > > Metrics lookups from map instead of using direct reference > > > > > (performance problem). > > > > > > > > 1. We(You and I) did this choice together to simplify creation of the > > new > > > > metrics. > > > > 2. This is not public API issue. > > > > > > > > > > > > > Ignoring of statisticsEnabled flag. > > > > > > > > I don’t ignore this flag. > > > > It just doesn’t exists in new framework(because of scope). > > > > I don’t think it’s an issue. > > > > > > > > > JmxExporterSpi creates beans that doesn't satisfy best MBeans > > > practices. > > > > > > > > Please, clarify your statement. > > > > What is best MBeans practices you are talking about? > > > > > > > > > Not finished IGNITE-11927 > > > > > > > > > > > > How this can be API issue? > > > > > > > > > > > > > 17 янв. 2020 г., в 20:52, Andrey Gura > написал(а): > > > > > > > > > >>> All issues Alexey mentioned in starting letter are fixed with my > PR > > > > [1]. > > > > >>> I don’t think other issues you mentioned are blocker for the > > release. > > > > > > > > > > As I mentioned already IGNITE-11927 is part of IEP-35 and > > > > > implementation seriously affects API's. Also I agree with Alexey > > about > > > > > introducing public IgniteMonitoring facade. So thiss PR doesn't fix > > > > > all issues. > > > > > > > > > >>> I talk about ignored existing functionality. > > > > >> There is no existing tests that was broken by this contribution. > > > > > > > > > > It's right. But if you add checking of statisticsEnabling property > > > > > then test will fail. It's just not good tests. > > > > > > > > > >> If you know the issues with it, feel free to create a ticket I > will > > > fix > > > > it ASAP. > > > > > > > > > > I already fix it all in IGNITE-11927 > > > > > > > > > >>> 1. Moving IEP-35 API's to the internal package. > > > > >> We should move the product forward and shouldn't hide major > > > > contribution from the user just because your second guess «I don’t > like > > > the > > > > API I just reviewed and approved». > > > > > > > > > > We should move the product forward with with really finished > > features, > > > > > not pieces of features. > > > > > > > > > >>
Re: Internal classes are exposed in public API
After giving it some thought, I agree with Denis - there is nothing wrong with exposing the new APIs, we just need to make it clear that we are still going to change it. Should we Introduce something like @IgniteExperimental annotation (I believe it has been discussed a dozen of times on the dev-list)? пт, 17 янв. 2020 г. в 23:28, Nikolay Izhikov : > +1 to mark feature or whole release as EA. > > пт, 17 янв. 2020 г., 23:00 Denis Magda : > > > Folks, if you don't mind I'll share some thoughts/suggestions as an > > observer who was not involved in the feature development. > > > > It's absolutely 'ok' to deprecate an API that is replaced with a much > > better version. However, we should do this only when the new APIs are > > production-ready. If there are still many limitations or open items then > > don't deprecate anything that exists and release the new APIs labeling as > > early access. What if release the improvements labeling as EA instead of > > hiding them completely? > > > > I would also encourage us to put aside emotions, don't blame or point > > fingers. This IEP is a great initiative and you both have already done a > > tremendous job by developing, architecting and reviewing changes. Just be > > respectful. Nobody is trying to block the feature from being released. > > Everyone would be glad to tap into improvements and start using them in > > prod. But if more time is needed for the GA let's reiterate a bit. > > > > - > > Denis > > > > > > On Fri, Jan 17, 2020 at 12:24 PM Николай Ижиков > > wrote: > > > > > > Also I agree with Alexey about introducing public IgniteMonitoring > > facade > > > > > > This is not an issue with the API. > > > Just the new feature that doesn’t affects API. > > > Moreover, I create a ticket to fix it, already. > > > > > > > It's right. But if you add checking of statisticsEnabling property > then > > > test will fail. It's just not good tests. > > > > > > My changes doesn’t affect any `staticticsEnabled` property. > > > I don’ understand your point here. > > > > > > > Redundant ReadOnlyMetricRegistry. > > > > > > It’s not redundant. > > > It required for exporters and provide read only access to > MetricRegistry > > > existing in the node. > > > > > > > > > > MetricExporterSpi that requires ReadOnlyMetricRegistry. > > > > > > > Absence of newly created metrics in old MBeans that forces user use > > > > exporter SPI while his code base uses old MBeans. > > > > > > Why this is issue? > > > > > > > Inconsistent MetricRegistry API. > > > > > > It’s consistent. > > > > > > > Metrics lookups from map instead of using direct reference > > > > (performance problem). > > > > > > 1. We(You and I) did this choice together to simplify creation of the > new > > > metrics. > > > 2. This is not public API issue. > > > > > > > > > > Ignoring of statisticsEnabled flag. > > > > > > I don’t ignore this flag. > > > It just doesn’t exists in new framework(because of scope). > > > I don’t think it’s an issue. > > > > > > > JmxExporterSpi creates beans that doesn't satisfy best MBeans > > practices. > > > > > > Please, clarify your statement. > > > What is best MBeans practices you are talking about? > > > > > > > Not finished IGNITE-11927 > > > > > > > > > How this can be API issue? > > > > > > > > > > 17 янв. 2020 г., в 20:52, Andrey Gura написал(а): > > > > > > > >>> All issues Alexey mentioned in starting letter are fixed with my PR > > > [1]. > > > >>> I don’t think other issues you mentioned are blocker for the > release. > > > > > > > > As I mentioned already IGNITE-11927 is part of IEP-35 and > > > > implementation seriously affects API's. Also I agree with Alexey > about > > > > introducing public IgniteMonitoring facade. So thiss PR doesn't fix > > > > all issues. > > > > > > > >>> I talk about ignored existing functionality. > > > >> There is no existing tests that was broken by this contribution. > > > > > > > > It's right. But if you add checking of statisticsEnabling property > > > > then test will fail. It's just not good tests. > > > > > > > >> If you know the issues with it, feel free to create a ticket I will > > fix > > > it ASAP. > > > > > > > > I already fix it all in IGNITE-11927 > > > > > > > >>> 1. Moving IEP-35 API's to the internal package. > > > >> We should move the product forward and shouldn't hide major > > > contribution from the user just because your second guess «I don’t like > > the > > > API I just reviewed and approved». > > > > > > > > We should move the product forward with with really finished > features, > > > > not pieces of features. > > > > > > > >> So I am against this proposal. > > > > > > > > It is not argument. > > > > > > > >> But, I’m ready to see your proposal for the API change and discuss > > them. > > > > > > > > We can prepare it together. But we can't block release. > > > > > > > >>> Because IGNITE-11927 doesn't solve all problems > > > >> What is *ALL* problems? > > > > > > > > Redundant ReadOnlyMetricRegistry. > > > >
Re: Internal classes are exposed in public API
+1 to mark feature or whole release as EA. пт, 17 янв. 2020 г., 23:00 Denis Magda : > Folks, if you don't mind I'll share some thoughts/suggestions as an > observer who was not involved in the feature development. > > It's absolutely 'ok' to deprecate an API that is replaced with a much > better version. However, we should do this only when the new APIs are > production-ready. If there are still many limitations or open items then > don't deprecate anything that exists and release the new APIs labeling as > early access. What if release the improvements labeling as EA instead of > hiding them completely? > > I would also encourage us to put aside emotions, don't blame or point > fingers. This IEP is a great initiative and you both have already done a > tremendous job by developing, architecting and reviewing changes. Just be > respectful. Nobody is trying to block the feature from being released. > Everyone would be glad to tap into improvements and start using them in > prod. But if more time is needed for the GA let's reiterate a bit. > > - > Denis > > > On Fri, Jan 17, 2020 at 12:24 PM Николай Ижиков > wrote: > > > > Also I agree with Alexey about introducing public IgniteMonitoring > facade > > > > This is not an issue with the API. > > Just the new feature that doesn’t affects API. > > Moreover, I create a ticket to fix it, already. > > > > > It's right. But if you add checking of statisticsEnabling property then > > test will fail. It's just not good tests. > > > > My changes doesn’t affect any `staticticsEnabled` property. > > I don’ understand your point here. > > > > > Redundant ReadOnlyMetricRegistry. > > > > It’s not redundant. > > It required for exporters and provide read only access to MetricRegistry > > existing in the node. > > > > > > > MetricExporterSpi that requires ReadOnlyMetricRegistry. > > > > > Absence of newly created metrics in old MBeans that forces user use > > > exporter SPI while his code base uses old MBeans. > > > > Why this is issue? > > > > > Inconsistent MetricRegistry API. > > > > It’s consistent. > > > > > Metrics lookups from map instead of using direct reference > > > (performance problem). > > > > 1. We(You and I) did this choice together to simplify creation of the new > > metrics. > > 2. This is not public API issue. > > > > > > > Ignoring of statisticsEnabled flag. > > > > I don’t ignore this flag. > > It just doesn’t exists in new framework(because of scope). > > I don’t think it’s an issue. > > > > > JmxExporterSpi creates beans that doesn't satisfy best MBeans > practices. > > > > Please, clarify your statement. > > What is best MBeans practices you are talking about? > > > > > Not finished IGNITE-11927 > > > > > > How this can be API issue? > > > > > > > 17 янв. 2020 г., в 20:52, Andrey Gura написал(а): > > > > > >>> All issues Alexey mentioned in starting letter are fixed with my PR > > [1]. > > >>> I don’t think other issues you mentioned are blocker for the release. > > > > > > As I mentioned already IGNITE-11927 is part of IEP-35 and > > > implementation seriously affects API's. Also I agree with Alexey about > > > introducing public IgniteMonitoring facade. So thiss PR doesn't fix > > > all issues. > > > > > >>> I talk about ignored existing functionality. > > >> There is no existing tests that was broken by this contribution. > > > > > > It's right. But if you add checking of statisticsEnabling property > > > then test will fail. It's just not good tests. > > > > > >> If you know the issues with it, feel free to create a ticket I will > fix > > it ASAP. > > > > > > I already fix it all in IGNITE-11927 > > > > > >>> 1. Moving IEP-35 API's to the internal package. > > >> We should move the product forward and shouldn't hide major > > contribution from the user just because your second guess «I don’t like > the > > API I just reviewed and approved». > > > > > > We should move the product forward with with really finished features, > > > not pieces of features. > > > > > >> So I am against this proposal. > > > > > > It is not argument. > > > > > >> But, I’m ready to see your proposal for the API change and discuss > them. > > > > > > We can prepare it together. But we can't block release. > > > > > >>> Because IGNITE-11927 doesn't solve all problems > > >> What is *ALL* problems? > > > > > > Redundant ReadOnlyMetricRegistry. > > > MetricExporterSpi that requires ReadOnlyMetricRegistry. > > > Absence of newly created metrics in old MBeans that forces user use > > > exporter SPI while his code base uses old MBeans. > > > Inconsistent MetricRegistry API. > > > Metrics lookups from map instead of using direct reference > > > (performance problem). > > > Ignoring of statisticsEnabled flag. > > > JmxExporterSpi creates beans that doesn't satisfy best MBeans > practices. > > > Not finished IGNITE-11927 > > > > > > It's enough I believe. > > > > > > On Fri, Jan 17, 2020 at 7:28 PM Николай Ижиков > > wrote: > > >> > > >> Andrey. > > >> > > >> All issues Alexey
Re: Internal classes are exposed in public API
Folks, if you don't mind I'll share some thoughts/suggestions as an observer who was not involved in the feature development. It's absolutely 'ok' to deprecate an API that is replaced with a much better version. However, we should do this only when the new APIs are production-ready. If there are still many limitations or open items then don't deprecate anything that exists and release the new APIs labeling as early access. What if release the improvements labeling as EA instead of hiding them completely? I would also encourage us to put aside emotions, don't blame or point fingers. This IEP is a great initiative and you both have already done a tremendous job by developing, architecting and reviewing changes. Just be respectful. Nobody is trying to block the feature from being released. Everyone would be glad to tap into improvements and start using them in prod. But if more time is needed for the GA let's reiterate a bit. - Denis On Fri, Jan 17, 2020 at 12:24 PM Николай Ижиков wrote: > > Also I agree with Alexey about introducing public IgniteMonitoring facade > > This is not an issue with the API. > Just the new feature that doesn’t affects API. > Moreover, I create a ticket to fix it, already. > > > It's right. But if you add checking of statisticsEnabling property then > test will fail. It's just not good tests. > > My changes doesn’t affect any `staticticsEnabled` property. > I don’ understand your point here. > > > Redundant ReadOnlyMetricRegistry. > > It’s not redundant. > It required for exporters and provide read only access to MetricRegistry > existing in the node. > > > > MetricExporterSpi that requires ReadOnlyMetricRegistry. > > > Absence of newly created metrics in old MBeans that forces user use > > exporter SPI while his code base uses old MBeans. > > Why this is issue? > > > Inconsistent MetricRegistry API. > > It’s consistent. > > > Metrics lookups from map instead of using direct reference > > (performance problem). > > 1. We(You and I) did this choice together to simplify creation of the new > metrics. > 2. This is not public API issue. > > > > Ignoring of statisticsEnabled flag. > > I don’t ignore this flag. > It just doesn’t exists in new framework(because of scope). > I don’t think it’s an issue. > > > JmxExporterSpi creates beans that doesn't satisfy best MBeans practices. > > Please, clarify your statement. > What is best MBeans practices you are talking about? > > > Not finished IGNITE-11927 > > > How this can be API issue? > > > > 17 янв. 2020 г., в 20:52, Andrey Gura написал(а): > > > >>> All issues Alexey mentioned in starting letter are fixed with my PR > [1]. > >>> I don’t think other issues you mentioned are blocker for the release. > > > > As I mentioned already IGNITE-11927 is part of IEP-35 and > > implementation seriously affects API's. Also I agree with Alexey about > > introducing public IgniteMonitoring facade. So thiss PR doesn't fix > > all issues. > > > >>> I talk about ignored existing functionality. > >> There is no existing tests that was broken by this contribution. > > > > It's right. But if you add checking of statisticsEnabling property > > then test will fail. It's just not good tests. > > > >> If you know the issues with it, feel free to create a ticket I will fix > it ASAP. > > > > I already fix it all in IGNITE-11927 > > > >>> 1. Moving IEP-35 API's to the internal package. > >> We should move the product forward and shouldn't hide major > contribution from the user just because your second guess «I don’t like the > API I just reviewed and approved». > > > > We should move the product forward with with really finished features, > > not pieces of features. > > > >> So I am against this proposal. > > > > It is not argument. > > > >> But, I’m ready to see your proposal for the API change and discuss them. > > > > We can prepare it together. But we can't block release. > > > >>> Because IGNITE-11927 doesn't solve all problems > >> What is *ALL* problems? > > > > Redundant ReadOnlyMetricRegistry. > > MetricExporterSpi that requires ReadOnlyMetricRegistry. > > Absence of newly created metrics in old MBeans that forces user use > > exporter SPI while his code base uses old MBeans. > > Inconsistent MetricRegistry API. > > Metrics lookups from map instead of using direct reference > > (performance problem). > > Ignoring of statisticsEnabled flag. > > JmxExporterSpi creates beans that doesn't satisfy best MBeans practices. > > Not finished IGNITE-11927 > > > > It's enough I believe. > > > > On Fri, Jan 17, 2020 at 7:28 PM Николай Ижиков > wrote: > >> > >> Andrey. > >> > >> All issues Alexey mentioned in starting letter are fixed with my PR [1]. > >> I don’t think other issues you mentioned are blocker for the release. > >> > >>> I talk about ignored existing functionality. > >> > >> There is no existing tests that was broken by this contribution. > >> If you know the issues with it, feel free to create a ticket I will fix > it ASAP. > >> > >>> 1. Moving
Re: Internal classes are exposed in public API
> Also I agree with Alexey about introducing public IgniteMonitoring facade This is not an issue with the API. Just the new feature that doesn’t affects API. Moreover, I create a ticket to fix it, already. > It's right. But if you add checking of statisticsEnabling property then test > will fail. It's just not good tests. My changes doesn’t affect any `staticticsEnabled` property. I don’ understand your point here. > Redundant ReadOnlyMetricRegistry. It’s not redundant. It required for exporters and provide read only access to MetricRegistry existing in the node. > MetricExporterSpi that requires ReadOnlyMetricRegistry. > Absence of newly created metrics in old MBeans that forces user use > exporter SPI while his code base uses old MBeans. Why this is issue? > Inconsistent MetricRegistry API. It’s consistent. > Metrics lookups from map instead of using direct reference > (performance problem). 1. We(You and I) did this choice together to simplify creation of the new metrics. 2. This is not public API issue. > Ignoring of statisticsEnabled flag. I don’t ignore this flag. It just doesn’t exists in new framework(because of scope). I don’t think it’s an issue. > JmxExporterSpi creates beans that doesn't satisfy best MBeans practices. Please, clarify your statement. What is best MBeans practices you are talking about? > Not finished IGNITE-11927 How this can be API issue? > 17 янв. 2020 г., в 20:52, Andrey Gura написал(а): > >>> All issues Alexey mentioned in starting letter are fixed with my PR [1]. >>> I don’t think other issues you mentioned are blocker for the release. > > As I mentioned already IGNITE-11927 is part of IEP-35 and > implementation seriously affects API's. Also I agree with Alexey about > introducing public IgniteMonitoring facade. So thiss PR doesn't fix > all issues. > >>> I talk about ignored existing functionality. >> There is no existing tests that was broken by this contribution. > > It's right. But if you add checking of statisticsEnabling property > then test will fail. It's just not good tests. > >> If you know the issues with it, feel free to create a ticket I will fix it >> ASAP. > > I already fix it all in IGNITE-11927 > >>> 1. Moving IEP-35 API's to the internal package. >> We should move the product forward and shouldn't hide major contribution >> from the user just because your second guess «I don’t like the API I just >> reviewed and approved». > > We should move the product forward with with really finished features, > not pieces of features. > >> So I am against this proposal. > > It is not argument. > >> But, I’m ready to see your proposal for the API change and discuss them. > > We can prepare it together. But we can't block release. > >>> Because IGNITE-11927 doesn't solve all problems >> What is *ALL* problems? > > Redundant ReadOnlyMetricRegistry. > MetricExporterSpi that requires ReadOnlyMetricRegistry. > Absence of newly created metrics in old MBeans that forces user use > exporter SPI while his code base uses old MBeans. > Inconsistent MetricRegistry API. > Metrics lookups from map instead of using direct reference > (performance problem). > Ignoring of statisticsEnabled flag. > JmxExporterSpi creates beans that doesn't satisfy best MBeans practices. > Not finished IGNITE-11927 > > It's enough I believe. > > On Fri, Jan 17, 2020 at 7:28 PM Николай Ижиков wrote: >> >> Andrey. >> >> All issues Alexey mentioned in starting letter are fixed with my PR [1]. >> I don’t think other issues you mentioned are blocker for the release. >> >>> I talk about ignored existing functionality. >> >> There is no existing tests that was broken by this contribution. >> If you know the issues with it, feel free to create a ticket I will fix it >> ASAP. >> >>> 1. Moving IEP-35 API's to the internal package. >> >> We should move the product forward and shouldn't hide major contribution >> from the user just because your second guess «I don’t like the API I just >> reviewed and approved». >> So I am against this proposal. >> But, I’m ready to see your proposal for the API change and discuss them. >> >>> Because IGNITE-11927 doesn't solve all problems >> >> What is *ALL* problems? >> Seems, we never be able to solve *ALL* problems. >> But, we should move the product forward. >> >> As for your steps [1-6]. >> I’m always following these steps during my contribution. >> >> [1] https://github.com/apache/ignite/pull/7269 >> >>> 17 янв. 2020 г., в 19:08, Andrey Gura написал(а): >>> >>> The discussion is hot and can be endless. So in separate post I want >>> propose my solution. >>> >>> 1. Moving IEP-35 API's to the internal package. >>> 2. Create special feature branch B. >>> 3. In branch B will be merged IGNITE-11927 >>> 4. Because IGNITE-11927 doesn't solve all problems we should propose >>> solution and implement it in branch B. >>> 5. Testing, usability testing, fixing, etc iteratively. >>> 6. Merge it to master and in new release
Re: Internal classes are exposed in public API
>> All issues Alexey mentioned in starting letter are fixed with my PR [1]. >> I don’t think other issues you mentioned are blocker for the release. As I mentioned already IGNITE-11927 is part of IEP-35 and implementation seriously affects API's. Also I agree with Alexey about introducing public IgniteMonitoring facade. So thiss PR doesn't fix all issues. >> I talk about ignored existing functionality. > There is no existing tests that was broken by this contribution. It's right. But if you add checking of statisticsEnabling property then test will fail. It's just not good tests. > If you know the issues with it, feel free to create a ticket I will fix it > ASAP. I already fix it all in IGNITE-11927 >> 1. Moving IEP-35 API's to the internal package. > We should move the product forward and shouldn't hide major contribution from > the user just because your second guess «I don’t like the API I just reviewed > and approved». We should move the product forward with with really finished features, not pieces of features. > So I am against this proposal. It is not argument. > But, I’m ready to see your proposal for the API change and discuss them. We can prepare it together. But we can't block release. >> Because IGNITE-11927 doesn't solve all problems > What is *ALL* problems? Redundant ReadOnlyMetricRegistry. MetricExporterSpi that requires ReadOnlyMetricRegistry. Absence of newly created metrics in old MBeans that forces user use exporter SPI while his code base uses old MBeans. Inconsistent MetricRegistry API. Metrics lookups from map instead of using direct reference (performance problem). Ignoring of statisticsEnabled flag. JmxExporterSpi creates beans that doesn't satisfy best MBeans practices. Not finished IGNITE-11927 It's enough I believe. On Fri, Jan 17, 2020 at 7:28 PM Николай Ижиков wrote: > > Andrey. > > All issues Alexey mentioned in starting letter are fixed with my PR [1]. > I don’t think other issues you mentioned are blocker for the release. > > > I talk about ignored existing functionality. > > There is no existing tests that was broken by this contribution. > If you know the issues with it, feel free to create a ticket I will fix it > ASAP. > > > 1. Moving IEP-35 API's to the internal package. > > We should move the product forward and shouldn't hide major contribution from > the user just because your second guess «I don’t like the API I just reviewed > and approved». > So I am against this proposal. > But, I’m ready to see your proposal for the API change and discuss them. > > > Because IGNITE-11927 doesn't solve all problems > > What is *ALL* problems? > Seems, we never be able to solve *ALL* problems. > But, we should move the product forward. > > As for your steps [1-6]. > I’m always following these steps during my contribution. > > [1] https://github.com/apache/ignite/pull/7269 > > > 17 янв. 2020 г., в 19:08, Andrey Gura написал(а): > > > > The discussion is hot and can be endless. So in separate post I want > > propose my solution. > > > > 1. Moving IEP-35 API's to the internal package. > > 2. Create special feature branch B. > > 3. In branch B will be merged IGNITE-11927 > > 4. Because IGNITE-11927 doesn't solve all problems we should propose > > solution and implement it in branch B. > > 5. Testing, usability testing, fixing, etc iteratively. > > 6. Merge it to master and in new release branch if needed. > > > > Independent step. There are some problem which should be fixed in 2.8 > > before release otherwise we introduce problems with compatibility > > which will haunt us till next major release. I'll create tickets. > > > > On Fri, Jan 17, 2020 at 7:03 PM Andrey Gura wrote: > >> > Because it is brand new API and it requires rewrite client code. > >>> We doesn’t break backward compatibility. > >>> The message is - this interface would be remove in the next major release. > >> > >> We don't know anything about development processes our users. I can > >> admit that process could require that deprecated methods/APIs are not > >> allowed for example. > >> > ReadOnlyMetricRegistry > Form user stand point it is very strange interface which don't give me > any information about it’s purpose and responsibilities. > Seems, I should explain proposed changes [1] more clear: > >> > >> I understand this. But I'm not Ignite user, I'm Ignite developer. The > >> key moment in my message *from user stand point*. From my point of > >> view it is very not intuitive solution and this interface is > >> redundant. > >> > Actually not. We have statisticsEnabled for caches for example. There > are other entities with such flag. > >>> They still works as expected. > >> > >> Actually not. I fixed many such issues during IGNITE-11927 implementation. > >> > Why do you decided do in such way? > >>> Because of the scope. > >>> The ability to disable/enable metrics is the matter of the other ticket. > >> > >> I talk not about ability. I talk
Re: Internal classes are exposed in public API
Andrey. All issues Alexey mentioned in starting letter are fixed with my PR [1]. I don’t think other issues you mentioned are blocker for the release. > I talk about ignored existing functionality. There is no existing tests that was broken by this contribution. If you know the issues with it, feel free to create a ticket I will fix it ASAP. > 1. Moving IEP-35 API's to the internal package. We should move the product forward and shouldn't hide major contribution from the user just because your second guess «I don’t like the API I just reviewed and approved». So I am against this proposal. But, I’m ready to see your proposal for the API change and discuss them. > Because IGNITE-11927 doesn't solve all problems What is *ALL* problems? Seems, we never be able to solve *ALL* problems. But, we should move the product forward. As for your steps [1-6]. I’m always following these steps during my contribution. [1] https://github.com/apache/ignite/pull/7269 > 17 янв. 2020 г., в 19:08, Andrey Gura написал(а): > > The discussion is hot and can be endless. So in separate post I want > propose my solution. > > 1. Moving IEP-35 API's to the internal package. > 2. Create special feature branch B. > 3. In branch B will be merged IGNITE-11927 > 4. Because IGNITE-11927 doesn't solve all problems we should propose > solution and implement it in branch B. > 5. Testing, usability testing, fixing, etc iteratively. > 6. Merge it to master and in new release branch if needed. > > Independent step. There are some problem which should be fixed in 2.8 > before release otherwise we introduce problems with compatibility > which will haunt us till next major release. I'll create tickets. > > On Fri, Jan 17, 2020 at 7:03 PM Andrey Gura wrote: >> Because it is brand new API and it requires rewrite client code. >>> We doesn’t break backward compatibility. >>> The message is - this interface would be remove in the next major release. >> >> We don't know anything about development processes our users. I can >> admit that process could require that deprecated methods/APIs are not >> allowed for example. >> ReadOnlyMetricRegistry Form user stand point it is very strange interface which don't give me any information about it’s purpose and responsibilities. Seems, I should explain proposed changes [1] more clear: >> >> I understand this. But I'm not Ignite user, I'm Ignite developer. The >> key moment in my message *from user stand point*. From my point of >> view it is very not intuitive solution and this interface is >> redundant. >> Actually not. We have statisticsEnabled for caches for example. There are other entities with such flag. >>> They still works as expected. >> >> Actually not. I fixed many such issues during IGNITE-11927 implementation. >> Why do you decided do in such way? >>> Because of the scope. >>> The ability to disable/enable metrics is the matter of the other ticket. >> >> I talk not about ability. I talk about ignored existing functionality. >> So scope is not case here. >> But they should not be exported by MetricExporterSpi implementations. >>> Actually, it’s a responsibility of the exporter to decide. >>> JMX exporter can exports ObjectMetric while OpenCensus exporter can’t. >> >> Actually list is not metric at all as I already told. >> >> On Fri, Jan 17, 2020 at 5:26 PM Николай Ижиков wrote: >>> >>> Andrey. >>> Because it is brand new API and it requires rewrite client code. >>> >>> We doesn’t break backward compatibility. >>> The message is - this interface would be remove in the next major release. >>> ReadOnlyMetricRegistry Form user stand point it is very strange interface which don't give me any information about it’s purpose and responsibilities. >>> >>> Seems, I should explain proposed changes [1] more clear: >>> >>> Each SPI would have a `ReadOnlyMetricManager` which provides access to >>> collection of `ReadOnlyMetricRegistry` >>> which has a collection of `Metric`. >>> >>> So we reflects two-level structure we have in the internal API >>> >>> GridMetricManager -> Collection[MetricRegistry] -> Collection[Metric] >>> ReadOnlyMetricManager -> Collection[ReadOnlyMetricRegistry] -> >>> Collection[Metric] >>> Actually not. We have statisticsEnabled for caches for example. There are other entities with such flag. >>> >>> They still works as expected. >>> Why do you decided do in such way? >>> >>> Because of the scope. >>> The ability to disable/enable metrics is the matter of the other ticket. >>> But they should not be exported by MetricExporterSpi implementations. >>> >>> Actually, it’s a responsibility of the exporter to decide. >>> JMX exporter can exports ObjectMetric while OpenCensus exporter can’t. >>> >>> [1] >>> https://github.com/apache/ignite/pull/7269/files#diff-0ae5657231fc4c1f650493b02190b81bR25 >>> 17 янв. 2020 г., в 16:57, Andrey Gura написал(а):
Re: Internal classes are exposed in public API
The discussion is hot and can be endless. So in separate post I want propose my solution. 1. Moving IEP-35 API's to the internal package. 2. Create special feature branch B. 3. In branch B will be merged IGNITE-11927 4. Because IGNITE-11927 doesn't solve all problems we should propose solution and implement it in branch B. 5. Testing, usability testing, fixing, etc iteratively. 6. Merge it to master and in new release branch if needed. Independent step. There are some problem which should be fixed in 2.8 before release otherwise we introduce problems with compatibility which will haunt us till next major release. I'll create tickets. On Fri, Jan 17, 2020 at 7:03 PM Andrey Gura wrote: > > >> Because it is brand new API and it requires rewrite client code. > > We doesn’t break backward compatibility. > > The message is - this interface would be remove in the next major release. > > We don't know anything about development processes our users. I can > admit that process could require that deprecated methods/APIs are not > allowed for example. > > >> ReadOnlyMetricRegistry > >> Form user stand point it is very strange interface which don't give me any > >> information about it’s purpose and responsibilities. > >> Seems, I should explain proposed changes [1] more clear: > > I understand this. But I'm not Ignite user, I'm Ignite developer. The > key moment in my message *from user stand point*. From my point of > view it is very not intuitive solution and this interface is > redundant. > > >> Actually not. We have statisticsEnabled for caches for example. There are > >> other entities with such flag. > > They still works as expected. > > Actually not. I fixed many such issues during IGNITE-11927 implementation. > > >> Why do you decided do in such way? > > Because of the scope. > > The ability to disable/enable metrics is the matter of the other ticket. > > I talk not about ability. I talk about ignored existing functionality. > So scope is not case here. > > >> But they should not be exported by MetricExporterSpi implementations. > > Actually, it’s a responsibility of the exporter to decide. > > JMX exporter can exports ObjectMetric while OpenCensus exporter can’t. > > Actually list is not metric at all as I already told. > > On Fri, Jan 17, 2020 at 5:26 PM Николай Ижиков wrote: > > > > Andrey. > > > > > Because it is brand new API and it requires rewrite client code. > > > > We doesn’t break backward compatibility. > > The message is - this interface would be remove in the next major release. > > > > > ReadOnlyMetricRegistry > > > Form user stand point it is very strange interface which don't give me > > > any information about it’s purpose and responsibilities. > > > > Seems, I should explain proposed changes [1] more clear: > > > > Each SPI would have a `ReadOnlyMetricManager` which provides access to > > collection of `ReadOnlyMetricRegistry` > > which has a collection of `Metric`. > > > > So we reflects two-level structure we have in the internal API > > > > GridMetricManager -> Collection[MetricRegistry] -> Collection[Metric] > > ReadOnlyMetricManager -> Collection[ReadOnlyMetricRegistry] -> > > Collection[Metric] > > > > > Actually not. We have statisticsEnabled for caches for example. There are > > > other entities with such flag. > > > > They still works as expected. > > > > > Why do you decided do in such way? > > > > Because of the scope. > > The ability to disable/enable metrics is the matter of the other ticket. > > > > > But they should not be exported by MetricExporterSpi implementations. > > > > Actually, it’s a responsibility of the exporter to decide. > > JMX exporter can exports ObjectMetric while OpenCensus exporter can’t. > > > > [1] > > https://github.com/apache/ignite/pull/7269/files#diff-0ae5657231fc4c1f650493b02190b81bR25 > > > > > 17 янв. 2020 г., в 16:57, Andrey Gura написал(а): > > > > > >> If I’m not missing something, you were one of the active reviewers of > > >> the Metric API. > > > > > > Yes. But if I'm not missing some thing you were major developer of > > > Metric API :) Shit happens. And it happened. > > > > > >>> The first, I agree with Alexey about deprecation of APIs that are still > > >>> supported and don't offer reasonable substitution. > > >> It has - MetricExporterSPI. > > > > > > There is such concept - backward compatibility. I understand that > > > deprecation of some interface doesn't break backward compatibility but > > > it leads to question^ what should I use instead of this. And > > > MetricExporterSpi is not answer for this question. Because it is brand > > > new API and it requires rewrite client code. > > > > > >>> ReadOnlyMetricRegistry interface is redundant. > > >> It’s an interface that exposes internal MetricRegistry to the exporters. > > > > > > No it is not. It's completely artificial thing which allow iterate via > > > all metric registries. GridMetricManager implements this interface > > > while it is not metric registry. Form
Re: Internal classes are exposed in public API
>> Because it is brand new API and it requires rewrite client code. > We doesn’t break backward compatibility. > The message is - this interface would be remove in the next major release. We don't know anything about development processes our users. I can admit that process could require that deprecated methods/APIs are not allowed for example. >> ReadOnlyMetricRegistry >> Form user stand point it is very strange interface which don't give me any >> information about it’s purpose and responsibilities. >> Seems, I should explain proposed changes [1] more clear: I understand this. But I'm not Ignite user, I'm Ignite developer. The key moment in my message *from user stand point*. From my point of view it is very not intuitive solution and this interface is redundant. >> Actually not. We have statisticsEnabled for caches for example. There are >> other entities with such flag. > They still works as expected. Actually not. I fixed many such issues during IGNITE-11927 implementation. >> Why do you decided do in such way? > Because of the scope. > The ability to disable/enable metrics is the matter of the other ticket. I talk not about ability. I talk about ignored existing functionality. So scope is not case here. >> But they should not be exported by MetricExporterSpi implementations. > Actually, it’s a responsibility of the exporter to decide. > JMX exporter can exports ObjectMetric while OpenCensus exporter can’t. Actually list is not metric at all as I already told. On Fri, Jan 17, 2020 at 5:26 PM Николай Ижиков wrote: > > Andrey. > > > Because it is brand new API and it requires rewrite client code. > > We doesn’t break backward compatibility. > The message is - this interface would be remove in the next major release. > > > ReadOnlyMetricRegistry > > Form user stand point it is very strange interface which don't give me any > > information about it’s purpose and responsibilities. > > Seems, I should explain proposed changes [1] more clear: > > Each SPI would have a `ReadOnlyMetricManager` which provides access to > collection of `ReadOnlyMetricRegistry` > which has a collection of `Metric`. > > So we reflects two-level structure we have in the internal API > > GridMetricManager -> Collection[MetricRegistry] -> Collection[Metric] > ReadOnlyMetricManager -> Collection[ReadOnlyMetricRegistry] -> > Collection[Metric] > > > Actually not. We have statisticsEnabled for caches for example. There are > > other entities with such flag. > > They still works as expected. > > > Why do you decided do in such way? > > Because of the scope. > The ability to disable/enable metrics is the matter of the other ticket. > > > But they should not be exported by MetricExporterSpi implementations. > > Actually, it’s a responsibility of the exporter to decide. > JMX exporter can exports ObjectMetric while OpenCensus exporter can’t. > > [1] > https://github.com/apache/ignite/pull/7269/files#diff-0ae5657231fc4c1f650493b02190b81bR25 > > > 17 янв. 2020 г., в 16:57, Andrey Gura написал(а): > > > >> If I’m not missing something, you were one of the active reviewers of the > >> Metric API. > > > > Yes. But if I'm not missing some thing you were major developer of > > Metric API :) Shit happens. And it happened. > > > >>> The first, I agree with Alexey about deprecation of APIs that are still > >>> supported and don't offer reasonable substitution. > >> It has - MetricExporterSPI. > > > > There is such concept - backward compatibility. I understand that > > deprecation of some interface doesn't break backward compatibility but > > it leads to question^ what should I use instead of this. And > > MetricExporterSpi is not answer for this question. Because it is brand > > new API and it requires rewrite client code. > > > >>> ReadOnlyMetricRegistry interface is redundant. > >> It’s an interface that exposes internal MetricRegistry to the exporters. > > > > No it is not. It's completely artificial thing which allow iterate via > > all metric registries. GridMetricManager implements this interface > > while it is not metric registry. Form user stand point it is very > > strange interface which don't give me any information about it's > > purpose and responsibilities. > > > >>> Exporters expose metrics if they are disabled. > >> We don’t have an ability to disable metrics. > > > > Actually not. We have statisticsEnabled for caches for example. There > > are other entities with such flag. > > > >> And that done, intentionally. > > > > Why do you decided do in such way? Why you ignore existing > > functionality? It affects user expectations and experience. > > > >> You are working on this issue, aren’t you? > > > > Yes? I'm working. Unfortunately we are not synchronized in this > > context and I should redo all metrics related changes in order to > > merge it with my changes. Anyway, my change doesn't solve all problems > > (e.g. it doesn't introduce IgniteMonitoring facade). > > > >> I can fix this issue, by myself. > > > >
Re: Internal classes are exposed in public API
Andrey. > Because it is brand new API and it requires rewrite client code. We doesn’t break backward compatibility. The message is - this interface would be remove in the next major release. > ReadOnlyMetricRegistry > Form user stand point it is very strange interface which don't give me any > information about it’s purpose and responsibilities. Seems, I should explain proposed changes [1] more clear: Each SPI would have a `ReadOnlyMetricManager` which provides access to collection of `ReadOnlyMetricRegistry` which has a collection of `Metric`. So we reflects two-level structure we have in the internal API GridMetricManager -> Collection[MetricRegistry] -> Collection[Metric] ReadOnlyMetricManager -> Collection[ReadOnlyMetricRegistry] -> Collection[Metric] > Actually not. We have statisticsEnabled for caches for example. There are > other entities with such flag. They still works as expected. > Why do you decided do in such way? Because of the scope. The ability to disable/enable metrics is the matter of the other ticket. > But they should not be exported by MetricExporterSpi implementations. Actually, it’s a responsibility of the exporter to decide. JMX exporter can exports ObjectMetric while OpenCensus exporter can’t. [1] https://github.com/apache/ignite/pull/7269/files#diff-0ae5657231fc4c1f650493b02190b81bR25 > 17 янв. 2020 г., в 16:57, Andrey Gura написал(а): > >> If I’m not missing something, you were one of the active reviewers of the >> Metric API. > > Yes. But if I'm not missing some thing you were major developer of > Metric API :) Shit happens. And it happened. > >>> The first, I agree with Alexey about deprecation of APIs that are still >>> supported and don't offer reasonable substitution. >> It has - MetricExporterSPI. > > There is such concept - backward compatibility. I understand that > deprecation of some interface doesn't break backward compatibility but > it leads to question^ what should I use instead of this. And > MetricExporterSpi is not answer for this question. Because it is brand > new API and it requires rewrite client code. > >>> ReadOnlyMetricRegistry interface is redundant. >> It’s an interface that exposes internal MetricRegistry to the exporters. > > No it is not. It's completely artificial thing which allow iterate via > all metric registries. GridMetricManager implements this interface > while it is not metric registry. Form user stand point it is very > strange interface which don't give me any information about it's > purpose and responsibilities. > >>> Exporters expose metrics if they are disabled. >> We don’t have an ability to disable metrics. > > Actually not. We have statisticsEnabled for caches for example. There > are other entities with such flag. > >> And that done, intentionally. > > Why do you decided do in such way? Why you ignore existing > functionality? It affects user expectations and experience. > >> You are working on this issue, aren’t you? > > Yes? I'm working. Unfortunately we are not synchronized in this > context and I should redo all metrics related changes in order to > merge it with my changes. Anyway, my change doesn't solve all problems > (e.g. it doesn't introduce IgniteMonitoring facade). > >> I can fix this issue, by myself. > > Unfortunately it will be just fix. In my solution it is redesign. Stop > fixing issues, let's do things. It requires deeper changes. My changes > blocks AI 2.8 release because it big enough. So it retargeted on the > next release. And it is one more reason for moving the changes to the > internal packages. And it isn't good news for me because I will go > throughout pan and tiers of merge. But it is right. > >> Metrics of type lists are not metric at all. > > They are created to deal with backward compatibility. > >>> Metrics of type lists are not metric at all. >> They are created to deal with backward compatibility. > > Yes, I know. But they should not be exported by MetricExporterSpi > implementations. > > On Fri, Jan 17, 2020 at 3:37 PM Николай Ижиков wrote: >> >> Andrey, thanks for your opinion and your ownest critisism. >> I can’t wait for your contribution. >> >> If I’m not missing something, you were one of the active reviewers of the >> Metric API. >> >>> The first, I agree with Alexey about deprecation of APIs that are still >>> supported and don't offer reasonable substitution. >> >> It has - MetricExporterSPI. >> >>> The second, from my point of view, we can't recommend MetricExporterSpi's >>> because it are still not-production ready. >> >> It’s ready. >> >>> The third, moving of MetricRegistry to the public API doesn't solve the >>> problem because this interface exposes internal Metric interface >>> implementations. >> >> Not, its’ not. >> Please, see `org.apache.ignite.spi.metric.LongMetric` and other public >> interface. >> >>> API of MetricRegistry is inconsistent. >> >> MetricRegistry is the internal API. >> Feel free to create ticket for
Re: Internal classes are exposed in public API
> If I’m not missing something, you were one of the active reviewers of the > Metric API. Yes. But if I'm not missing some thing you were major developer of Metric API :) Shit happens. And it happened. >> The first, I agree with Alexey about deprecation of APIs that are still >> supported and don't offer reasonable substitution. > It has - MetricExporterSPI. There is such concept - backward compatibility. I understand that deprecation of some interface doesn't break backward compatibility but it leads to question^ what should I use instead of this. And MetricExporterSpi is not answer for this question. Because it is brand new API and it requires rewrite client code. >> ReadOnlyMetricRegistry interface is redundant. > It’s an interface that exposes internal MetricRegistry to the exporters. No it is not. It's completely artificial thing which allow iterate via all metric registries. GridMetricManager implements this interface while it is not metric registry. Form user stand point it is very strange interface which don't give me any information about it's purpose and responsibilities. >> Exporters expose metrics if they are disabled. > We don’t have an ability to disable metrics. Actually not. We have statisticsEnabled for caches for example. There are other entities with such flag. > And that done, intentionally. Why do you decided do in such way? Why you ignore existing functionality? It affects user expectations and experience. > You are working on this issue, aren’t you? Yes? I'm working. Unfortunately we are not synchronized in this context and I should redo all metrics related changes in order to merge it with my changes. Anyway, my change doesn't solve all problems (e.g. it doesn't introduce IgniteMonitoring facade). > I can fix this issue, by myself. Unfortunately it will be just fix. In my solution it is redesign. Stop fixing issues, let's do things. It requires deeper changes. My changes blocks AI 2.8 release because it big enough. So it retargeted on the next release. And it is one more reason for moving the changes to the internal packages. And it isn't good news for me because I will go throughout pan and tiers of merge. But it is right. > Metrics of type lists are not metric at all. They are created to deal with backward compatibility. >> Metrics of type lists are not metric at all. > They are created to deal with backward compatibility. Yes, I know. But they should not be exported by MetricExporterSpi implementations. On Fri, Jan 17, 2020 at 3:37 PM Николай Ижиков wrote: > > Andrey, thanks for your opinion and your ownest critisism. > I can’t wait for your contribution. > > If I’m not missing something, you were one of the active reviewers of the > Metric API. > > > The first, I agree with Alexey about deprecation of APIs that are still > > supported and don't offer reasonable substitution. > > It has - MetricExporterSPI. > > > The second, from my point of view, we can't recommend MetricExporterSpi's > > because it are still not-production ready. > > It’s ready. > > > The third, moving of MetricRegistry to the public API doesn't solve the > > problem because this interface exposes internal Metric interface > > implementations. > > Not, its’ not. > Please, see `org.apache.ignite.spi.metric.LongMetric` and other public > interface. > > > API of MetricRegistry is inconsistent. > > MetricRegistry is the internal API. > Feel free to create ticket for an issues with it and I will try to fix it. > > > ReadOnlyMetricRegistry interface is redundant. > > It’s an interface that exposes internal MetricRegistry to the exporters. > > > Exporters expose metrics if they are disabled. > > We don’t have an ability to disable metrics. > And that done, intentionally. > You are working on this issue, aren’t you? > I can fix this issue, by myself. > > > Metrics of type lists are not metric at all. > > They are created to deal with backward compatibility. > > > 17 янв. 2020 г., в 15:09, Andrey Gura написал(а): > > > > Hi, > > > > The first, I agree with Alexey about deprecation of APIs that are > > still supported and don't offer reasonable substitution. > > > > The second, from my point of view, we can't recommend > > MetricExporterSpi's because it are still not-production ready. There > > are some issues with it and usage of ReadOnlyMetricRegistry interface > > just one of them. > > > > The third, moving of MetricRegistry to the public API doesn't solve > > the problem because this interface exposes internal Metric interface > > implementations. So your PR is incomplete. > > Moreover, API of MetricRegistry is inconsistent. E.g. register(name, > > supplier, desc) method returns registered metric for some types and > > doesn't for other. register(metric) method is inconsistent in sense of > > metric naming. ReadOnlyMetricRegistry interface is redundant. > > MetricExporterSpi should be revised because it absolutely not > > intuitive because it requires ReadOnlyMetricRegistry and it's
Re: Internal classes are exposed in public API
Andrey, thanks for your opinion and your ownest critisism. I can’t wait for your contribution. If I’m not missing something, you were one of the active reviewers of the Metric API. > The first, I agree with Alexey about deprecation of APIs that are still > supported and don't offer reasonable substitution. It has - MetricExporterSPI. > The second, from my point of view, we can't recommend MetricExporterSpi's > because it are still not-production ready. It’s ready. > The third, moving of MetricRegistry to the public API doesn't solve the > problem because this interface exposes internal Metric interface > implementations. Not, its’ not. Please, see `org.apache.ignite.spi.metric.LongMetric` and other public interface. > API of MetricRegistry is inconsistent. MetricRegistry is the internal API. Feel free to create ticket for an issues with it and I will try to fix it. > ReadOnlyMetricRegistry interface is redundant. It’s an interface that exposes internal MetricRegistry to the exporters. > Exporters expose metrics if they are disabled. We don’t have an ability to disable metrics. And that done, intentionally. You are working on this issue, aren’t you? I can fix this issue, by myself. > Metrics of type lists are not metric at all. They are created to deal with backward compatibility. > 17 янв. 2020 г., в 15:09, Andrey Gura написал(а): > > Hi, > > The first, I agree with Alexey about deprecation of APIs that are > still supported and don't offer reasonable substitution. > > The second, from my point of view, we can't recommend > MetricExporterSpi's because it are still not-production ready. There > are some issues with it and usage of ReadOnlyMetricRegistry interface > just one of them. > > The third, moving of MetricRegistry to the public API doesn't solve > the problem because this interface exposes internal Metric interface > implementations. So your PR is incomplete. > Moreover, API of MetricRegistry is inconsistent. E.g. register(name, > supplier, desc) method returns registered metric for some types and > doesn't for other. register(metric) method is inconsistent in sense of > metric naming. ReadOnlyMetricRegistry interface is redundant. > MetricExporterSpi should be revised because it absolutely not > intuitive because it requires ReadOnlyMetricRegistry and it's purpose > is undefined. > > One more point. IEP-35 is still not fully implemented. Some things are > not taken into account. Exporters expose metrics if they are disabled. > JMX beans exposes values that don't confirm to best practices [1]. > Metrics of type lists are not metric at all. Ubiquitous merics lookup > from hash map instead of usage reference for getting metrics values > (it is just performance issue). Also IGNITE-11927 is not implemented > which also changes interfaces significantly. > > Let's just admit that the implementation is immature and must be moved > to the internal packages. > > And because we already merged partially implemented IEP to the master > branch we *must move all currently public APIs to the internal API* > while it will not be ready for publication. > > And the last but not least. What is happening indicates a immature > development process which must be revised. I don't want discuss it in > this thread but we must not allow merge of change to the master branch > before it will completed, that is we must use feature branches for > full IEP not only for particular tickets. And also we should > reformulate IEP process in order to avoid things like this. > > [1] > https://www.oracle.com/technetwork/java/javase/tech/best-practices-jsp-136021.html > > On Fri, Jan 17, 2020 at 12:49 PM Николай Ижиков wrote: >> >> Alex. >> >> OK, I may leverage your experience and create pure Java API. >> Ticket [1] created. >> >> But, personally, I don’t agree with you. >> Ignite has dozens of the API that theoretically have a usage scenario, but >> in real-world have 0 custom implementation and usages. >> Moreover, many APIs that were created with the intentions you mentioned is >> abandoned now and confuses users. >> >> You can just see count of the tests we just mute on the TC. >> >> Can you, please, take a look at the fix regarding puck API issue you >> mentioned in your first letter [2], [3] >> >> [1] https://issues.apache.org/jira/browse/IGNITE-12553 >> [2] https://github.com/apache/ignite/pull/7269 >> [3] https://issues.apache.org/jira/browse/IGNITE-12552 >> >> >>> 17 янв. 2020 г., в 12:12, Alexey Goncharuk >>> написал(а): >>> >>> Nikolay, >>> >>> Why do you think this is a wrong usage pattern? From the top of my head, >>> here is a few cases of direct metric API usage that I know are currently >>> being used in production: >>> * A custom task execution scheduling service with load balancing based on >>> utilization metrics readings from Java code >>> * Cleanup task trigger based on metrics readings >>> * A custom health-check endpoint for an application with an embedded >>>
Re: Internal classes are exposed in public API
Hi, The first, I agree with Alexey about deprecation of APIs that are still supported and don't offer reasonable substitution. The second, from my point of view, we can't recommend MetricExporterSpi's because it are still not-production ready. There are some issues with it and usage of ReadOnlyMetricRegistry interface just one of them. The third, moving of MetricRegistry to the public API doesn't solve the problem because this interface exposes internal Metric interface implementations. So your PR is incomplete. Moreover, API of MetricRegistry is inconsistent. E.g. register(name, supplier, desc) method returns registered metric for some types and doesn't for other. register(metric) method is inconsistent in sense of metric naming. ReadOnlyMetricRegistry interface is redundant. MetricExporterSpi should be revised because it absolutely not intuitive because it requires ReadOnlyMetricRegistry and it's purpose is undefined. One more point. IEP-35 is still not fully implemented. Some things are not taken into account. Exporters expose metrics if they are disabled. JMX beans exposes values that don't confirm to best practices [1]. Metrics of type lists are not metric at all. Ubiquitous merics lookup from hash map instead of usage reference for getting metrics values (it is just performance issue). Also IGNITE-11927 is not implemented which also changes interfaces significantly. Let's just admit that the implementation is immature and must be moved to the internal packages. And because we already merged partially implemented IEP to the master branch we *must move all currently public APIs to the internal API* while it will not be ready for publication. And the last but not least. What is happening indicates a immature development process which must be revised. I don't want discuss it in this thread but we must not allow merge of change to the master branch before it will completed, that is we must use feature branches for full IEP not only for particular tickets. And also we should reformulate IEP process in order to avoid things like this. [1] https://www.oracle.com/technetwork/java/javase/tech/best-practices-jsp-136021.html On Fri, Jan 17, 2020 at 12:49 PM Николай Ижиков wrote: > > Alex. > > OK, I may leverage your experience and create pure Java API. > Ticket [1] created. > > But, personally, I don’t agree with you. > Ignite has dozens of the API that theoretically have a usage scenario, but in > real-world have 0 custom implementation and usages. > Moreover, many APIs that were created with the intentions you mentioned is > abandoned now and confuses users. > > You can just see count of the tests we just mute on the TC. > > Can you, please, take a look at the fix regarding puck API issue you > mentioned in your first letter [2], [3] > > [1] https://issues.apache.org/jira/browse/IGNITE-12553 > [2] https://github.com/apache/ignite/pull/7269 > [3] https://issues.apache.org/jira/browse/IGNITE-12552 > > > > 17 янв. 2020 г., в 12:12, Alexey Goncharuk > > написал(а): > > > > Nikolay, > > > > Why do you think this is a wrong usage pattern? From the top of my head, > > here is a few cases of direct metric API usage that I know are currently > > being used in production: > > * A custom task execution scheduling service with load balancing based on > > utilization metrics readings from Java code > > * Cleanup task trigger based on metrics readings > > * A custom health-check endpoint for an application with an embedded > > Ignite node for Kubernetes/Spring Application/etc >
Re: Internal classes are exposed in public API
Nikolay, thanks! I am not insisting we must implement IGNITE-12553 right away nor am I saying it's mandatory. I just want to make sure we do not deprecate existing APIs unless we as a community are certain that they will be dropped and there is no need for the replacement. Will take a look at the PR shortly. пт, 17 янв. 2020 г. в 12:49, Николай Ижиков : > Alex. > > OK, I may leverage your experience and create pure Java API. > Ticket [1] created. > > But, personally, I don’t agree with you. > Ignite has dozens of the API that theoretically have a usage scenario, but > in real-world have 0 custom implementation and usages. > Moreover, many APIs that were created with the intentions you mentioned is > abandoned now and confuses users. > > You can just see count of the tests we just mute on the TC. > > Can you, please, take a look at the fix regarding puck API issue you > mentioned in your first letter [2], [3] > > [1] https://issues.apache.org/jira/browse/IGNITE-12553 > [2] https://github.com/apache/ignite/pull/7269 > [3] https://issues.apache.org/jira/browse/IGNITE-12552 > > > > 17 янв. 2020 г., в 12:12, Alexey Goncharuk > написал(а): > > > > Nikolay, > > > > Why do you think this is a wrong usage pattern? From the top of my head, > > here is a few cases of direct metric API usage that I know are currently > > being used in production: > > * A custom task execution scheduling service with load balancing based on > > utilization metrics readings from Java code > > * Cleanup task trigger based on metrics readings > > * A custom health-check endpoint for an application with an embedded > > Ignite node for Kubernetes/Spring Application/etc > >
Re: Internal classes are exposed in public API
Alex. OK, I may leverage your experience and create pure Java API. Ticket [1] created. But, personally, I don’t agree with you. Ignite has dozens of the API that theoretically have a usage scenario, but in real-world have 0 custom implementation and usages. Moreover, many APIs that were created with the intentions you mentioned is abandoned now and confuses users. You can just see count of the tests we just mute on the TC. Can you, please, take a look at the fix regarding puck API issue you mentioned in your first letter [2], [3] [1] https://issues.apache.org/jira/browse/IGNITE-12553 [2] https://github.com/apache/ignite/pull/7269 [3] https://issues.apache.org/jira/browse/IGNITE-12552 > 17 янв. 2020 г., в 12:12, Alexey Goncharuk > написал(а): > > Nikolay, > > Why do you think this is a wrong usage pattern? From the top of my head, > here is a few cases of direct metric API usage that I know are currently > being used in production: > * A custom task execution scheduling service with load balancing based on > utilization metrics readings from Java code > * Cleanup task trigger based on metrics readings > * A custom health-check endpoint for an application with an embedded > Ignite node for Kubernetes/Spring Application/etc
Re: Internal classes are exposed in public API
Nikolay, Why do you think this is a wrong usage pattern? From the top of my head, here is a few cases of direct metric API usage that I know are currently being used in production: * A custom task execution scheduling service with load balancing based on utilization metrics readings from Java code * Cleanup task trigger based on metrics readings * A custom health-check endpoint for an application with an embedded Ignite node for Kubernetes/Spring Application/etc
Re: Internal classes are exposed in public API
Pavel,
As I said before - I don’t see any real use-cases for your example.
That’s why
> I don’t create pure Java API for metric intentionally.
What real-world use-case you are keeping in mind?
Why do you want to do it in that way?
The API you’re talking about is very simple and straightforward.
It’s an easy thing to create, but this will open to the user's wrong usage
pattern.
From my point of view, to get metrics values one should use MetricExporterSPI.
> 17 янв. 2020 г., в 09:40, Pavel Tupitsyn написал(а):
>
> Николай, can you provide a full example - how do I get a metric value in my
> code as an Ignite user?
>
> var ignite = Ignition.ignite();
> var totalAllocatedPages = ???;
>
> On Fri, Jan 17, 2020 at 12:47 AM Николай Ижиков wrote:
>
>> Hello, Pavel.
>>
>>> there should be an obvious public API to retrieve metrics
>>
>> What do you mean by «retrieve»?
>> I don’t create pure Java API for metric intentionally.
>> We have MetricExporterSPI for this purpose.
>> It very simple from my point of view.
>> What use cases for `Ignite.monitoring()` API exists?
>>
>> ```
>> public interface MetricExporterSpi extends IgniteSpi {
>>public void setMetricRegistry(ReadOnlyMetricRegistry registry);
>>public void setExportFilter(Predicate filter);
>> }
>> ```
>>
>> ```
>> public interface ReadOnlyMetricRegistry extends Iterable {
>>public void addMetricRegistryCreationListener(Consumer
>> lsnr);
>>public void addMetricRegistryRemoveListener(Consumer
>> lsnr);
>> }
>> ```
>>
>> ```
>> public class MetricRegistry implements Iterable {
>>public String name();
>>@Nullable public M findMetric(String name);
>> }
>> ```
>>
>>> 16 янв. 2020 г., в 20:32, Pavel Tupitsyn
>> написал(а):
>>>
>>> Agree with Alexey, there should be an obvious public API to retrieve
>> metrics
>>>
>>> On Thu, Jan 16, 2020 at 8:23 PM Alexey Goncharuk <
>> alexey.goncha...@gmail.com>
>>> wrote:
>>>
That's an option, I agree. Yet for me, from the usability point of
>> view, it
would be quite strange - to get an instance of the monitoring
>> interface, I
would need to do
JmxMetricExporterSpi metrics =
(JmxMetricExporterSpi)ignite.configuration().getMetricExporterSpi()[0];
which is too verbose and fragile if someone changes configuration (say,
inserts another SPI to the first position).
чт, 16 янв. 2020 г. в 20:14, Николай Ижиков :
> May be we should enable JMX exporter, by default?
> This would provide the same value for the user as `IgniteMonitoring`
>> you
> propose.
>
>> 16 янв. 2020 г., в 20:06, Alexey Goncharuk <
>> alexey.goncha...@gmail.com
>
> написал(а):
>>
>> I think it would make sense to make something like a new
IgniteMonitoring
>> facade on Ignite interface and add an ability to obtain a metric value
>> through this facade, not through an exporter. This would be a
>> straightforward replacement for all old metrics interfaces.
>>
>> чт, 16 янв. 2020 г. в 17:13, Николай Ижиков :
>>
>>> Ticket to export MetricRegistry to the public API created -
>>> https://issues.apache.org/jira/browse/IGNITE-12552
>>>
16 янв. 2020 г., в 16:58, Николай Ижиков
>>> написал(а):
Do you propose to keep these interfaces forever?
> 16 янв. 2020 г., в 16:52, Alexey Goncharuk <
> alexey.goncha...@gmail.com>
>>> написал(а):
>
> Let's say I am upgrading from 2.7 to 2.8. Given that I figured out
to
> obtain an instance of the JMX exporter SPI, how should I map the
>> old
> 'interface + method name' to the new metric name? I think
deprecation
> of
> the public API may be a bit harsh in this case.
>
> чт, 16 янв. 2020 г. в 16:44, Николай Ижиков :
>
>> Hello, Alexey.
>>
>>> * DataRegionMetric public interface is deprecated, however, the
>> suggested replacement class GridMetricsManager is internal and
cannot
>>> be
>> acquired by a user. This makes impossible for the user to fix
>> their
>>> code to
>> not use deprecated API
>>
>> May be it’s not clear text here.
>> My point here, that user should obtain metrics values via
configured
>> metric exporters and don’t use *Metric interfaces.
>>
>>> * In ReadOnlyMetricsRegistry there is a Consumer,
> but
>> MetricRegistry is again an internal class.
>>
>> This is a real issue.
>> We should expose MetricRegistry interface to the public API and
keep
>> internal implementation.
>>
>> I will create ticket and fix it, shortly.
>>
>>
>>> 16 янв. 2020 г., в 16:39, Alexey Goncharuk <
>>> alexey.goncha...@gmail.com>
>> написал(а):
>>>
>>> Igniters, Nikolay,
>>>
>>> I
Re: Internal classes are exposed in public API
Hello, Pavel.
> there should be an obvious public API to retrieve metrics
What do you mean by «retrieve»?
I don’t create pure Java API for metric intentionally.
We have MetricExporterSPI for this purpose.
It very simple from my point of view.
What use cases for `Ignite.monitoring()` API exists?
```
public interface MetricExporterSpi extends IgniteSpi {
public void setMetricRegistry(ReadOnlyMetricRegistry registry);
public void setExportFilter(Predicate filter);
}
```
```
public interface ReadOnlyMetricRegistry extends Iterable {
public void addMetricRegistryCreationListener(Consumer
lsnr);
public void addMetricRegistryRemoveListener(Consumer lsnr);
}
```
```
public class MetricRegistry implements Iterable {
public String name();
@Nullable public M findMetric(String name);
}
```
> 16 янв. 2020 г., в 20:32, Pavel Tupitsyn написал(а):
>
> Agree with Alexey, there should be an obvious public API to retrieve metrics
>
> On Thu, Jan 16, 2020 at 8:23 PM Alexey Goncharuk
> wrote:
>
>> That's an option, I agree. Yet for me, from the usability point of view, it
>> would be quite strange - to get an instance of the monitoring interface, I
>> would need to do
>> JmxMetricExporterSpi metrics =
>> (JmxMetricExporterSpi)ignite.configuration().getMetricExporterSpi()[0];
>> which is too verbose and fragile if someone changes configuration (say,
>> inserts another SPI to the first position).
>>
>> чт, 16 янв. 2020 г. в 20:14, Николай Ижиков :
>>
>>> May be we should enable JMX exporter, by default?
>>> This would provide the same value for the user as `IgniteMonitoring` you
>>> propose.
>>>
16 янв. 2020 г., в 20:06, Alexey Goncharuk >>
>>> написал(а):
I think it would make sense to make something like a new
>> IgniteMonitoring
facade on Ignite interface and add an ability to obtain a metric value
through this facade, not through an exporter. This would be a
straightforward replacement for all old metrics interfaces.
чт, 16 янв. 2020 г. в 17:13, Николай Ижиков :
> Ticket to export MetricRegistry to the public API created -
> https://issues.apache.org/jira/browse/IGNITE-12552
>
>> 16 янв. 2020 г., в 16:58, Николай Ижиков
> написал(а):
>>
>> Do you propose to keep these interfaces forever?
>>
>>> 16 янв. 2020 г., в 16:52, Alexey Goncharuk <
>>> alexey.goncha...@gmail.com>
> написал(а):
>>>
>>> Let's say I am upgrading from 2.7 to 2.8. Given that I figured out
>> to
>>> obtain an instance of the JMX exporter SPI, how should I map the old
>>> 'interface + method name' to the new metric name? I think
>> deprecation
>>> of
>>> the public API may be a bit harsh in this case.
>>>
>>> чт, 16 янв. 2020 г. в 16:44, Николай Ижиков :
>>>
Hello, Alexey.
> * DataRegionMetric public interface is deprecated, however, the
suggested replacement class GridMetricsManager is internal and
>> cannot
> be
acquired by a user. This makes impossible for the user to fix their
> code to
not use deprecated API
May be it’s not clear text here.
My point here, that user should obtain metrics values via
>> configured
metric exporters and don’t use *Metric interfaces.
> * In ReadOnlyMetricsRegistry there is a Consumer,
>>> but
MetricRegistry is again an internal class.
This is a real issue.
We should expose MetricRegistry interface to the public API and
>> keep
internal implementation.
I will create ticket and fix it, shortly.
> 16 янв. 2020 г., в 16:39, Alexey Goncharuk <
> alexey.goncha...@gmail.com>
написал(а):
>
> Igniters, Nikolay,
>
> I was adding a new metric in the scope of the ticket [1] and was
surprised by a few things:
> * DataRegionMetric public interface is deprecated, however, the
suggested replacement class GridMetricsManager is internal and
>> cannot
> be
acquired by a user. This makes impossible for the user to fix their
> code to
not use deprecated API
> * In ReadOnlyMetricsRegistry there is a Consumer,
>>> but
MetricRegistry is again an internal class. This will cause the user
> code
compilation to break when MetricRegistry is changed
>
> These things violate the public-private API separation and need to
>>> be
fixed prior 2.8 is released. Let's discuss what changes need to be
> made to
the API or perhaps move incomplete IEP-35 interfaces to the private
> package
and remove deprecations until the API is ready.
>
> --AG
>>
>
>
>>>
>>>
>>
Re: Internal classes are exposed in public API
Agree with Alexey, there should be an obvious public API to retrieve metrics On Thu, Jan 16, 2020 at 8:23 PM Alexey Goncharuk wrote: > That's an option, I agree. Yet for me, from the usability point of view, it > would be quite strange - to get an instance of the monitoring interface, I > would need to do > JmxMetricExporterSpi metrics = > (JmxMetricExporterSpi)ignite.configuration().getMetricExporterSpi()[0]; > which is too verbose and fragile if someone changes configuration (say, > inserts another SPI to the first position). > > чт, 16 янв. 2020 г. в 20:14, Николай Ижиков : > > > May be we should enable JMX exporter, by default? > > This would provide the same value for the user as `IgniteMonitoring` you > > propose. > > > > > 16 янв. 2020 г., в 20:06, Alexey Goncharuk > > > написал(а): > > > > > > I think it would make sense to make something like a new > IgniteMonitoring > > > facade on Ignite interface and add an ability to obtain a metric value > > > through this facade, not through an exporter. This would be a > > > straightforward replacement for all old metrics interfaces. > > > > > > чт, 16 янв. 2020 г. в 17:13, Николай Ижиков : > > > > > >> Ticket to export MetricRegistry to the public API created - > > >> https://issues.apache.org/jira/browse/IGNITE-12552 > > >> > > >>> 16 янв. 2020 г., в 16:58, Николай Ижиков > > >> написал(а): > > >>> > > >>> Do you propose to keep these interfaces forever? > > >>> > > 16 янв. 2020 г., в 16:52, Alexey Goncharuk < > > alexey.goncha...@gmail.com> > > >> написал(а): > > > > Let's say I am upgrading from 2.7 to 2.8. Given that I figured out > to > > obtain an instance of the JMX exporter SPI, how should I map the old > > 'interface + method name' to the new metric name? I think > deprecation > > of > > the public API may be a bit harsh in this case. > > > > чт, 16 янв. 2020 г. в 16:44, Николай Ижиков : > > > > > Hello, Alexey. > > > > > >> * DataRegionMetric public interface is deprecated, however, the > > > suggested replacement class GridMetricsManager is internal and > cannot > > >> be > > > acquired by a user. This makes impossible for the user to fix their > > >> code to > > > not use deprecated API > > > > > > May be it’s not clear text here. > > > My point here, that user should obtain metrics values via > configured > > > metric exporters and don’t use *Metric interfaces. > > > > > >> * In ReadOnlyMetricsRegistry there is a Consumer, > > but > > > MetricRegistry is again an internal class. > > > > > > This is a real issue. > > > We should expose MetricRegistry interface to the public API and > keep > > > internal implementation. > > > > > > I will create ticket and fix it, shortly. > > > > > > > > >> 16 янв. 2020 г., в 16:39, Alexey Goncharuk < > > >> alexey.goncha...@gmail.com> > > > написал(а): > > >> > > >> Igniters, Nikolay, > > >> > > >> I was adding a new metric in the scope of the ticket [1] and was > > > surprised by a few things: > > >> * DataRegionMetric public interface is deprecated, however, the > > > suggested replacement class GridMetricsManager is internal and > cannot > > >> be > > > acquired by a user. This makes impossible for the user to fix their > > >> code to > > > not use deprecated API > > >> * In ReadOnlyMetricsRegistry there is a Consumer, > > but > > > MetricRegistry is again an internal class. This will cause the user > > >> code > > > compilation to break when MetricRegistry is changed > > >> > > >> These things violate the public-private API separation and need to > > be > > > fixed prior 2.8 is released. Let's discuss what changes need to be > > >> made to > > > the API or perhaps move incomplete IEP-35 interfaces to the private > > >> package > > > and remove deprecations until the API is ready. > > >> > > >> --AG > > > > > > > > >>> > > >> > > >> > > > > >
Re: Internal classes are exposed in public API
That's an option, I agree. Yet for me, from the usability point of view, it would be quite strange - to get an instance of the monitoring interface, I would need to do JmxMetricExporterSpi metrics = (JmxMetricExporterSpi)ignite.configuration().getMetricExporterSpi()[0]; which is too verbose and fragile if someone changes configuration (say, inserts another SPI to the first position). чт, 16 янв. 2020 г. в 20:14, Николай Ижиков : > May be we should enable JMX exporter, by default? > This would provide the same value for the user as `IgniteMonitoring` you > propose. > > > 16 янв. 2020 г., в 20:06, Alexey Goncharuk > написал(а): > > > > I think it would make sense to make something like a new IgniteMonitoring > > facade on Ignite interface and add an ability to obtain a metric value > > through this facade, not through an exporter. This would be a > > straightforward replacement for all old metrics interfaces. > > > > чт, 16 янв. 2020 г. в 17:13, Николай Ижиков : > > > >> Ticket to export MetricRegistry to the public API created - > >> https://issues.apache.org/jira/browse/IGNITE-12552 > >> > >>> 16 янв. 2020 г., в 16:58, Николай Ижиков > >> написал(а): > >>> > >>> Do you propose to keep these interfaces forever? > >>> > 16 янв. 2020 г., в 16:52, Alexey Goncharuk < > alexey.goncha...@gmail.com> > >> написал(а): > > Let's say I am upgrading from 2.7 to 2.8. Given that I figured out to > obtain an instance of the JMX exporter SPI, how should I map the old > 'interface + method name' to the new metric name? I think deprecation > of > the public API may be a bit harsh in this case. > > чт, 16 янв. 2020 г. в 16:44, Николай Ижиков : > > > Hello, Alexey. > > > >> * DataRegionMetric public interface is deprecated, however, the > > suggested replacement class GridMetricsManager is internal and cannot > >> be > > acquired by a user. This makes impossible for the user to fix their > >> code to > > not use deprecated API > > > > May be it’s not clear text here. > > My point here, that user should obtain metrics values via configured > > metric exporters and don’t use *Metric interfaces. > > > >> * In ReadOnlyMetricsRegistry there is a Consumer, > but > > MetricRegistry is again an internal class. > > > > This is a real issue. > > We should expose MetricRegistry interface to the public API and keep > > internal implementation. > > > > I will create ticket and fix it, shortly. > > > > > >> 16 янв. 2020 г., в 16:39, Alexey Goncharuk < > >> alexey.goncha...@gmail.com> > > написал(а): > >> > >> Igniters, Nikolay, > >> > >> I was adding a new metric in the scope of the ticket [1] and was > > surprised by a few things: > >> * DataRegionMetric public interface is deprecated, however, the > > suggested replacement class GridMetricsManager is internal and cannot > >> be > > acquired by a user. This makes impossible for the user to fix their > >> code to > > not use deprecated API > >> * In ReadOnlyMetricsRegistry there is a Consumer, > but > > MetricRegistry is again an internal class. This will cause the user > >> code > > compilation to break when MetricRegistry is changed > >> > >> These things violate the public-private API separation and need to > be > > fixed prior 2.8 is released. Let's discuss what changes need to be > >> made to > > the API or perhaps move incomplete IEP-35 interfaces to the private > >> package > > and remove deprecations until the API is ready. > >> > >> --AG > > > > > >>> > >> > >> > >
Re: Internal classes are exposed in public API
May be we should enable JMX exporter, by default? This would provide the same value for the user as `IgniteMonitoring` you propose. > 16 янв. 2020 г., в 20:06, Alexey Goncharuk > написал(а): > > I think it would make sense to make something like a new IgniteMonitoring > facade on Ignite interface and add an ability to obtain a metric value > through this facade, not through an exporter. This would be a > straightforward replacement for all old metrics interfaces. > > чт, 16 янв. 2020 г. в 17:13, Николай Ижиков : > >> Ticket to export MetricRegistry to the public API created - >> https://issues.apache.org/jira/browse/IGNITE-12552 >> >>> 16 янв. 2020 г., в 16:58, Николай Ижиков >> написал(а): >>> >>> Do you propose to keep these interfaces forever? >>> 16 янв. 2020 г., в 16:52, Alexey Goncharuk >> написал(а): Let's say I am upgrading from 2.7 to 2.8. Given that I figured out to obtain an instance of the JMX exporter SPI, how should I map the old 'interface + method name' to the new metric name? I think deprecation of the public API may be a bit harsh in this case. чт, 16 янв. 2020 г. в 16:44, Николай Ижиков : > Hello, Alexey. > >> * DataRegionMetric public interface is deprecated, however, the > suggested replacement class GridMetricsManager is internal and cannot >> be > acquired by a user. This makes impossible for the user to fix their >> code to > not use deprecated API > > May be it’s not clear text here. > My point here, that user should obtain metrics values via configured > metric exporters and don’t use *Metric interfaces. > >> * In ReadOnlyMetricsRegistry there is a Consumer, but > MetricRegistry is again an internal class. > > This is a real issue. > We should expose MetricRegistry interface to the public API and keep > internal implementation. > > I will create ticket and fix it, shortly. > > >> 16 янв. 2020 г., в 16:39, Alexey Goncharuk < >> alexey.goncha...@gmail.com> > написал(а): >> >> Igniters, Nikolay, >> >> I was adding a new metric in the scope of the ticket [1] and was > surprised by a few things: >> * DataRegionMetric public interface is deprecated, however, the > suggested replacement class GridMetricsManager is internal and cannot >> be > acquired by a user. This makes impossible for the user to fix their >> code to > not use deprecated API >> * In ReadOnlyMetricsRegistry there is a Consumer, but > MetricRegistry is again an internal class. This will cause the user >> code > compilation to break when MetricRegistry is changed >> >> These things violate the public-private API separation and need to be > fixed prior 2.8 is released. Let's discuss what changes need to be >> made to > the API or perhaps move incomplete IEP-35 interfaces to the private >> package > and remove deprecations until the API is ready. >> >> --AG > > >>> >> >>
Re: Internal classes are exposed in public API
I think it would make sense to make something like a new IgniteMonitoring facade on Ignite interface and add an ability to obtain a metric value through this facade, not through an exporter. This would be a straightforward replacement for all old metrics interfaces. чт, 16 янв. 2020 г. в 17:13, Николай Ижиков : > Ticket to export MetricRegistry to the public API created - > https://issues.apache.org/jira/browse/IGNITE-12552 > > > 16 янв. 2020 г., в 16:58, Николай Ижиков > написал(а): > > > > Do you propose to keep these interfaces forever? > > > >> 16 янв. 2020 г., в 16:52, Alexey Goncharuk > написал(а): > >> > >> Let's say I am upgrading from 2.7 to 2.8. Given that I figured out to > >> obtain an instance of the JMX exporter SPI, how should I map the old > >> 'interface + method name' to the new metric name? I think deprecation of > >> the public API may be a bit harsh in this case. > >> > >> чт, 16 янв. 2020 г. в 16:44, Николай Ижиков : > >> > >>> Hello, Alexey. > >>> > * DataRegionMetric public interface is deprecated, however, the > >>> suggested replacement class GridMetricsManager is internal and cannot > be > >>> acquired by a user. This makes impossible for the user to fix their > code to > >>> not use deprecated API > >>> > >>> May be it’s not clear text here. > >>> My point here, that user should obtain metrics values via configured > >>> metric exporters and don’t use *Metric interfaces. > >>> > * In ReadOnlyMetricsRegistry there is a Consumer, but > >>> MetricRegistry is again an internal class. > >>> > >>> This is a real issue. > >>> We should expose MetricRegistry interface to the public API and keep > >>> internal implementation. > >>> > >>> I will create ticket and fix it, shortly. > >>> > >>> > 16 янв. 2020 г., в 16:39, Alexey Goncharuk < > alexey.goncha...@gmail.com> > >>> написал(а): > > Igniters, Nikolay, > > I was adding a new metric in the scope of the ticket [1] and was > >>> surprised by a few things: > * DataRegionMetric public interface is deprecated, however, the > >>> suggested replacement class GridMetricsManager is internal and cannot > be > >>> acquired by a user. This makes impossible for the user to fix their > code to > >>> not use deprecated API > * In ReadOnlyMetricsRegistry there is a Consumer, but > >>> MetricRegistry is again an internal class. This will cause the user > code > >>> compilation to break when MetricRegistry is changed > > These things violate the public-private API separation and need to be > >>> fixed prior 2.8 is released. Let's discuss what changes need to be > made to > >>> the API or perhaps move incomplete IEP-35 interfaces to the private > package > >>> and remove deprecations until the API is ready. > > --AG > >>> > >>> > > > >
Re: Internal classes are exposed in public API
Ticket to export MetricRegistry to the public API created - https://issues.apache.org/jira/browse/IGNITE-12552 > 16 янв. 2020 г., в 16:58, Николай Ижиков написал(а): > > Do you propose to keep these interfaces forever? > >> 16 янв. 2020 г., в 16:52, Alexey Goncharuk >> написал(а): >> >> Let's say I am upgrading from 2.7 to 2.8. Given that I figured out to >> obtain an instance of the JMX exporter SPI, how should I map the old >> 'interface + method name' to the new metric name? I think deprecation of >> the public API may be a bit harsh in this case. >> >> чт, 16 янв. 2020 г. в 16:44, Николай Ижиков : >> >>> Hello, Alexey. >>> * DataRegionMetric public interface is deprecated, however, the >>> suggested replacement class GridMetricsManager is internal and cannot be >>> acquired by a user. This makes impossible for the user to fix their code to >>> not use deprecated API >>> >>> May be it’s not clear text here. >>> My point here, that user should obtain metrics values via configured >>> metric exporters and don’t use *Metric interfaces. >>> * In ReadOnlyMetricsRegistry there is a Consumer, but >>> MetricRegistry is again an internal class. >>> >>> This is a real issue. >>> We should expose MetricRegistry interface to the public API and keep >>> internal implementation. >>> >>> I will create ticket and fix it, shortly. >>> >>> 16 янв. 2020 г., в 16:39, Alexey Goncharuk >>> написал(а): Igniters, Nikolay, I was adding a new metric in the scope of the ticket [1] and was >>> surprised by a few things: * DataRegionMetric public interface is deprecated, however, the >>> suggested replacement class GridMetricsManager is internal and cannot be >>> acquired by a user. This makes impossible for the user to fix their code to >>> not use deprecated API * In ReadOnlyMetricsRegistry there is a Consumer, but >>> MetricRegistry is again an internal class. This will cause the user code >>> compilation to break when MetricRegistry is changed These things violate the public-private API separation and need to be >>> fixed prior 2.8 is released. Let's discuss what changes need to be made to >>> the API or perhaps move incomplete IEP-35 interfaces to the private package >>> and remove deprecations until the API is ready. --AG >>> >>> >
Re: Internal classes are exposed in public API
Do you propose to keep these interfaces forever? > 16 янв. 2020 г., в 16:52, Alexey Goncharuk > написал(а): > > Let's say I am upgrading from 2.7 to 2.8. Given that I figured out to > obtain an instance of the JMX exporter SPI, how should I map the old > 'interface + method name' to the new metric name? I think deprecation of > the public API may be a bit harsh in this case. > > чт, 16 янв. 2020 г. в 16:44, Николай Ижиков : > >> Hello, Alexey. >> >>> * DataRegionMetric public interface is deprecated, however, the >> suggested replacement class GridMetricsManager is internal and cannot be >> acquired by a user. This makes impossible for the user to fix their code to >> not use deprecated API >> >> May be it’s not clear text here. >> My point here, that user should obtain metrics values via configured >> metric exporters and don’t use *Metric interfaces. >> >>> * In ReadOnlyMetricsRegistry there is a Consumer, but >> MetricRegistry is again an internal class. >> >> This is a real issue. >> We should expose MetricRegistry interface to the public API and keep >> internal implementation. >> >> I will create ticket and fix it, shortly. >> >> >>> 16 янв. 2020 г., в 16:39, Alexey Goncharuk >> написал(а): >>> >>> Igniters, Nikolay, >>> >>> I was adding a new metric in the scope of the ticket [1] and was >> surprised by a few things: >>> * DataRegionMetric public interface is deprecated, however, the >> suggested replacement class GridMetricsManager is internal and cannot be >> acquired by a user. This makes impossible for the user to fix their code to >> not use deprecated API >>> * In ReadOnlyMetricsRegistry there is a Consumer, but >> MetricRegistry is again an internal class. This will cause the user code >> compilation to break when MetricRegistry is changed >>> >>> These things violate the public-private API separation and need to be >> fixed prior 2.8 is released. Let's discuss what changes need to be made to >> the API or perhaps move incomplete IEP-35 interfaces to the private package >> and remove deprecations until the API is ready. >>> >>> --AG >> >>
Re: Internal classes are exposed in public API
Let's say I am upgrading from 2.7 to 2.8. Given that I figured out to obtain an instance of the JMX exporter SPI, how should I map the old 'interface + method name' to the new metric name? I think deprecation of the public API may be a bit harsh in this case. чт, 16 янв. 2020 г. в 16:44, Николай Ижиков : > Hello, Alexey. > > > * DataRegionMetric public interface is deprecated, however, the > suggested replacement class GridMetricsManager is internal and cannot be > acquired by a user. This makes impossible for the user to fix their code to > not use deprecated API > > May be it’s not clear text here. > My point here, that user should obtain metrics values via configured > metric exporters and don’t use *Metric interfaces. > > > * In ReadOnlyMetricsRegistry there is a Consumer, but > MetricRegistry is again an internal class. > > This is a real issue. > We should expose MetricRegistry interface to the public API and keep > internal implementation. > > I will create ticket and fix it, shortly. > > > > 16 янв. 2020 г., в 16:39, Alexey Goncharuk > написал(а): > > > > Igniters, Nikolay, > > > > I was adding a new metric in the scope of the ticket [1] and was > surprised by a few things: > > * DataRegionMetric public interface is deprecated, however, the > suggested replacement class GridMetricsManager is internal and cannot be > acquired by a user. This makes impossible for the user to fix their code to > not use deprecated API > > * In ReadOnlyMetricsRegistry there is a Consumer, but > MetricRegistry is again an internal class. This will cause the user code > compilation to break when MetricRegistry is changed > > > > These things violate the public-private API separation and need to be > fixed prior 2.8 is released. Let's discuss what changes need to be made to > the API or perhaps move incomplete IEP-35 interfaces to the private package > and remove deprecations until the API is ready. > > > > --AG > >
Re: Internal classes are exposed in public API
Hello, Alexey. > * DataRegionMetric public interface is deprecated, however, the suggested > replacement class GridMetricsManager is internal and cannot be acquired by a > user. This makes impossible for the user to fix their code to not use > deprecated API May be it’s not clear text here. My point here, that user should obtain metrics values via configured metric exporters and don’t use *Metric interfaces. > * In ReadOnlyMetricsRegistry there is a Consumer, but > MetricRegistry is again an internal class. This is a real issue. We should expose MetricRegistry interface to the public API and keep internal implementation. I will create ticket and fix it, shortly. > 16 янв. 2020 г., в 16:39, Alexey Goncharuk > написал(а): > > Igniters, Nikolay, > > I was adding a new metric in the scope of the ticket [1] and was surprised by > a few things: > * DataRegionMetric public interface is deprecated, however, the suggested > replacement class GridMetricsManager is internal and cannot be acquired by a > user. This makes impossible for the user to fix their code to not use > deprecated API > * In ReadOnlyMetricsRegistry there is a Consumer, but > MetricRegistry is again an internal class. This will cause the user code > compilation to break when MetricRegistry is changed > > These things violate the public-private API separation and need to be fixed > prior 2.8 is released. Let's discuss what changes need to be made to the API > or perhaps move incomplete IEP-35 interfaces to the private package and > remove deprecations until the API is ready. > > --AG
Re: Internal classes are exposed in public API
The ticket [1] was https://issues.apache.org/jira/browse/IGNITE-12550 чт, 16 янв. 2020 г. в 16:39, Alexey Goncharuk : > Igniters, Nikolay, > > I was adding a new metric in the scope of the ticket [1] and was surprised > by a few things: > * DataRegionMetric public interface is deprecated, however, the suggested > replacement class GridMetricsManager is internal and cannot be acquired by > a user. This makes impossible for the user to fix their code to not use > deprecated API > * In ReadOnlyMetricsRegistry there is a Consumer, but > MetricRegistry is again an internal class. This will cause the user code > compilation to break when MetricRegistry is changed > > These things violate the public-private API separation and need to be > fixed prior 2.8 is released. Let's discuss what changes need to be made to > the API or perhaps move incomplete IEP-35 interfaces to the private package > and remove deprecations until the API is ready. > > --AG >
Internal classes are exposed in public API
Igniters, Nikolay, I was adding a new metric in the scope of the ticket [1] and was surprised by a few things: * DataRegionMetric public interface is deprecated, however, the suggested replacement class GridMetricsManager is internal and cannot be acquired by a user. This makes impossible for the user to fix their code to not use deprecated API * In ReadOnlyMetricsRegistry there is a Consumer, but MetricRegistry is again an internal class. This will cause the user code compilation to break when MetricRegistry is changed These things violate the public-private API separation and need to be fixed prior 2.8 is released. Let's discuss what changes need to be made to the API or perhaps move incomplete IEP-35 interfaces to the private package and remove deprecations until the API is ready. --AG
