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