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