> 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 > >>>>>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>>> > >>>>>>>>>>>>>> > >>>>>>>>>>>>> > >>>>>>>>>> > >>>>>>>>>> > >>>>>>>> > >>>>>>>> > >>>>>> > >>>>> > >>>> > >> > >> >
