Hello, Igniters.
* IGNITE-12552: Move ReadOnlyMetricRegistry to public API merged to the master
and cherry-picked to the 2.8.
So the main issue with the Metric API solved.
* I raised the PR [1] to fix second issue with the new Metric API: absence of
the public Java API to get metrics.
This PR introduces the following changes:
1. IgniteMetric interface created: it provides Java API to access Ignite
metrics created with the new Metric API.
```
public interface IgniteMetric extends Iterable<ReadOnlyMetricRegistry> {
@Nullable ReadOnlyMetricRegistry registry(String name);
}
```
2. All deprecation javadocs regarding metrics now reference to the public
IgniteMetric instead of internal GridMetricManager:
> @deprecated Use {@link IgniteMetric} instead.
3. Tests refactored to use IgniteMetric instead of GridMetricManager when
possible.
Please, review.
[1] https://github.com/apache/ignite/pull/7283
> 21 янв. 2020 г., в 17:51, Николай Ижиков <nizhikov....@gmail.com> написал(а):
>
> Hello, Igniters.
>
> Alexey approved my PR [1] regarding fixing public API for metric exporters.
> I’m waiting for a bot visa and merge this PR.
>
> As we discussed, the metrics API will be marked with IgniteExperimental
> annotation.
>
> If anyone has any objection to this plan, please provide your feedback.
>
> [1] https://github.com/apache/ignite/pull/7269
>
>> 21 янв. 2020 г., в 13:45, Николай Ижиков <nizhikov....@gmail.com> написал(а):
>>
>> 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
>>>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>>>
>>>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>>
>>>>>>>>>>
>>>>>>>>>
>>>>>>
>>>>>>
>>>>
>>>>
>>
>
