ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Николай Ижиков" <nizhi...@apache.org>
Subject Re: Internal classes are exposed in public API
Date Fri, 17 Jan 2020 14:26:07 GMT
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 <agura@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 Николай Ижиков <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