>> 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.
I disagree. It's part of API and it affects user experience. > Moreover, I create a ticket to fix it, already. Creating an issue does not solve the problem. >> 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? Because it is user experience. As I wrote already user must configure additional SPI in order to gt just a couple of numbers. It is not good idea from point of view. >> Inconsistent MetricRegistry API. > It’s consistent. I described concrete problems with API consistency early in this thread. Jus saying "it's consistent" doesn't make it 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. No. I pointed to this problem. > 2. This is not public API issue. But this is issue. >> Ignoring of statisticsEnabled flag. > I don’t ignore this flag. But flag's value is ignored. > It just doesn’t exists in new framework(because of scope). I don't understand how this scope was formulated. There is some feature. It didn't removed. So it should be taken into account during adding new functionality if it affected. > I don’t think it’s an issue. It's just opinion. Form my point of view it is bug. >> JmxExporterSpi creates beans that doesn't satisfy best MBeans practices. > Please, clarify your statement. > What is best MBeans practices you are talking about? Again. I provided link to the article about it early in this thread. >> Not finished IGNITE-11927 > How this can be API issue? As I wrote early in this thread (may be twice) it changes the API significantly. On Fri, Jan 17, 2020 at 9: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 > >>>>>>>>> > >>>>>>> > >>>>> > >> >
