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