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