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