Andrey. With this API we are trying to fill the GAP Alexey pointed out at the start of this discussion. I think it worth to be reviewed and merged.
Can we, please, come back to the discussion of the changes itself? I think API changes should be discussed in the other thread. > 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 [1] https://github.com/apache/ignite/pull/7283 > 24 янв. 2020 г., в 18:08, Andrey Gura <ag...@apache.org> написал(а): > >> 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. > > Argument is not about time. But 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 Николай Ижиков <nizhi...@apache.org> 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 <ag...@apache.org> написал(а): >>> >>>> 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 Николай Ижиков <nizhi...@apache.org> 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 <ag...@apache.org> написал(а): >>>>> >>>>> 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 Iterable<ReadOnlyMetricRegistry> { >>>>>> @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, Николай Ижиков <nizhikov....@gmail.com> >>>>>>> написал(а): >>>>>>> >>>>>>> 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, Николай Ижиков <nizhikov....@gmail.com> >>>>>>>> написал(а): >>>>>>>> >>>>>>>> Thanks, for the review Alexey. >>>>>>>> >>>>>>>> I will fix your comment and try to implement Monitoring facade, >>>>>>>> shortly. >>>>>>>> >>>>>>>>> 21 янв. 2020 г., в 13:32, Alexey Goncharuk >>>>>>>>> <alexey.goncha...@gmail.com> написал(а): >>>>>>>>> >>>>>>>>> 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, Николай Ижиков <nizhi...@apache.org>: >>>>>>>>> >>>>>>>>>> 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 >>>>>>>>>>> <alexey.goncha...@gmail.com> >>>>>>>>>> написал(а): >>>>>>>>>>> >>>>>>>>>>> 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, Николай Ижиков <nizhi...@apache.org>: >>>>>>>>>>> >>>>>>>>>>>> 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 <ag...@apache.org> >>>>>>>>>>>>> написал(а): >>>>>>>>>>>>> >>>>>>>>>>>>> It solves problem. >>>>>>>>>>>>> >>>>>>>>>>>>> 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 >>>>>>>>>>>>>> <nizhi...@apache.org>: >>>>>>>>>>>>>> >>>>>>>>>>>>>>> +1 to mark feature or whole release as EA. >>>>>>>>>>>>>>> >>>>>>>>>>>>>>> пт, 17 янв. 2020 г., 23:00 Denis Magda <dma...@apache.org>: >>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> 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. >>>>>>>>>>>>>>>>> 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 <ag...@apache.org> >>>>>>>>>>>> написал(а): >>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> 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 Николай Ижиков < >>>>>>>>>> nizhi...@apache.org >>>>>>>>>>>>> >>>>>>>>>>>>>>>>> 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 <ag...@apache.org> >>>>>>>>>>>>>>> написал(а): >>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>> 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 >>>>>>>>>>>>>>>>>>>> <ag...@apache.org> >>>>>>>>>>>>>>>> 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 Николай Ижиков < >>>>>>>>>>>>>>> nizhi...@apache.org >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> 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 <ag...@apache.org> >>>>>>>>>>>>>>>>> написал(а): >>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>> 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 Николай Ижиков < >>>>>>>>>>>>>>>> nizhi...@apache.org> >>>>>>>>>>>>>>>>> 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 >>>>>>>>>>>>>>>>>>>>>>>>> <ag...@apache.org> >>>>>>>>>>>>>>>>> написал(а): >>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>> 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 Николай Ижиков < >>>>>>>>>>>>>>>>> nizhi...@apache.org> 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 < >>>>>>>>>>>>>>>>> alexey.goncha...@gmail.com> написал(а): >>>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>>>>> 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 >>>>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>>> >>>>>>>>>>>>>>>> >>>>>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>> >>>>>>> >>>>>> >>>> >>>> >>
