Thanks, for the review Alexey.

I will fix your comment and  try to implement Monitoring facade, shortly.

> 21 янв. 2020 г., в 13:32, Alexey Goncharuk <alexey.goncha...@gmail.com> 
> написал(а):
> 
> Nikolay,
> 
> I left a single comment in the PR about the histogram metric. I think the
> API looks much cleaner now.
> 
> I will take care of the @IgniteExperimental annotation.
> 
> пн, 20 янв. 2020 г. в 20:55, Николай Ижиков <nizhi...@apache.org>:
> 
>> Alexey.
>> 
>> PR [1] is waiting for your review.
>> Please, take a look.
>> 
>> I think we should do the following before 2.8 release
>> 
>> * Introduce new @IgniteExperimental annotation as discussed.
>> * Mark Monitoring API with it.
>> * merge «[IEP-35] Expose MetricRegistry to the public API» to 2.8
>> * merge «[IEP-35] public Java metric API» to 2.8
>> 
>> [1] https://github.com/apache/ignite/pull/7269
>> 
>>> 20 янв. 2020 г., в 17:09, Alexey Goncharuk <alexey.goncha...@gmail.com>
>> написал(а):
>>> 
>>> Nikolay,
>>> 
>>> Should we wait for both of the tickets given that we agreed that we are
>>> putting an experimental marker on the new APIs? I'm ok to fix only the
>>> first one and add the experimental marker so that we do not delay 2.8
>>> release.
>>> 
>>> пн, 20 янв. 2020 г. в 13:32, Николай Ижиков <nizhi...@apache.org>:
>>> 
>>>> Andrey.
>>>> 
>>>> Let’s move from the long letters to the code.
>>>> If you want to change API - please, propose this changes.
>>>> I think everybody wins if we make our API better.
>>>> 
>>>> Please, describe proposed changes.
>>>> It would be great if you have some examples or PR for it.
>>>> 
>>>> As for this release, I have plans to contribute tickets
>>>> «[IEP-35] Expose MetricRegistry to the public API» [1] and
>>>> «[IEP-35] public Java metric API» [2] for it.
>>>> 
>>>> Any objections on it?
>>>> 
>>>> [1] https://github.com/apache/ignite/pull/7269
>>>> [2] https://issues.apache.org/jira/browse/IGNITE-12553
>>>> 
>>>> 
>>>>> 20 янв. 2020 г., в 13:08, Andrey Gura <ag...@apache.org> написал(а):
>>>>> 
>>>>> It solves problem.
>>>>> 
>>>>> On Mon, Jan 20, 2020 at 12:09 PM Alexey Goncharuk
>>>>> <alexey.goncha...@gmail.com> wrote:
>>>>>> 
>>>>>> After giving it some thought, I agree with Denis - there is nothing
>>>> wrong
>>>>>> with exposing the new APIs, we just need to make it clear that we are
>>>> still
>>>>>> going to change it.
>>>>>> 
>>>>>> Should we Introduce something like @IgniteExperimental annotation (I
>>>>>> believe it has been discussed a dozen of times on the dev-list)?
>>>>>> 
>>>>>> пт, 17 янв. 2020 г. в 23:28, Nikolay Izhikov <nizhi...@apache.org>:
>>>>>> 
>>>>>>> +1 to mark feature or whole release as EA.
>>>>>>> 
>>>>>>> пт, 17 янв. 2020 г., 23:00 Denis Magda <dma...@apache.org>:
>>>>>>> 
>>>>>>>> Folks, if you don't mind I'll share some thoughts/suggestions as an
>>>>>>>> observer who was not involved in the feature development.
>>>>>>>> 
>>>>>>>> It's absolutely 'ok' to deprecate an API that is replaced with a
>> much
>>>>>>>> better version. However, we should do this only when the new APIs
>> are
>>>>>>>> production-ready. If there are still many limitations or open items
>>>> then
>>>>>>>> don't deprecate anything that exists and release the new APIs
>>>> labeling as
>>>>>>>> early access. What if release the improvements labeling as EA
>> instead
>>>> of
>>>>>>>> hiding them completely?
>>>>>>>> 
>>>>>>>> I would also encourage us to put aside emotions, don't blame or
>> point
>>>>>>>> fingers. This IEP is a great initiative and you both have already
>>>> done a
>>>>>>>> tremendous job by developing, architecting and reviewing changes.
>>>> Just be
>>>>>>>> respectful. Nobody is trying to block the feature from being
>> released.
>>>>>>>> Everyone would be glad to tap into improvements and start using them
>>>> in
>>>>>>>> prod. But if more time is needed for the GA let's reiterate a bit.
>>>>>>>> 
>>>>>>>> -
>>>>>>>> Denis
>>>>>>>> 
>>>>>>>> 
>>>>>>>> On Fri, Jan 17, 2020 at 12: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
>>>>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>>>> 
>>>>>>>>>>>>>> 
>>>>>>>>>>> 
>>>>>>>>> 
>>>>>>>>> 
>>>>>>>> 
>>>>>>> 
>>>> 
>>>> 
>> 
>> 

Reply via email to