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