Folks,
I'm sorry for not being too attentive to the whole thread discussion and maybe missing some details. But... who will perform the review of [1] issue? Will we merge it to 2.8? (hope so) [1] https://issues.apache.org/jira/browse/IGNITE-12553 [IEP-35] public Java metric API On Sat, 25 Jan 2020 at 11:43, Николай Ижиков <nizhi...@apache.org> wrote: > > 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 > >>>>>>>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> > >>>>>>>>>>>> > >>>>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>> > >>>>>>> > >>>>>> > >>>> > >>>> > >> >
