Nikolay, I've reviewed your changes and I tend to agree with Andrey, it's better to remove deprecation for the old APIs in AI 2.8 and discuss and merge the new facade (IGNITE-12553) in a more calm and structured way. My observations on the changes:
- I am not sure if Iterable would suffice both for the exporter and public APIs. Should we provide a way to know the number of metrics and registries in advance? Let's say I am writing a custom exporter in binary format and I need to write the number of exported metrics in a packet beforehand. - There is no separation on public and internal metrics, when public metrics preserve their name and semantics, and internal metrics may change. I remember we discussed this, but it's not reflected in the APIs in any way. - Will we allow users to register their own metrics? Again I remember we discussed a possibility for this. If so, why the registry is called 'ReadyOnly'? - It's still not clear how a user will map old interfaces and methods to the new metric names. As you can see in the original message, I am not pushing for the new APIs in the nearest release, I just want to remove the deprecations on the classes for which there is no a well-defined replacement. пн, 27 янв. 2020 г. в 15:56, Maxim Muzafarov <mmu...@apache.org>: > 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 > > >>>>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>>> > > >>>>>>>>>>>>>>> > > >>>>>>>>>>>> > > >>>>>>>>>>>> > > >>>>>>>>>> > > >>>>>>>>>> > > >>>>>>>> > > >>>>>>> > > >>>>>> > > >>>> > > >>>> > > >> > > >
