ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Andrey Gura <ag...@apache.org>
Subject Re: Internal classes are exposed in public API
Date Fri, 17 Jan 2020 13:57:33 GMT
> 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 Николай Ижиков <nizhikov@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 <agura@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 Николай Ижиков <nizhikov@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.goncharuk@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
> >>
>

Mime
View raw message