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