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