+1 to @IgniteExperimental 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 > > > > >>>>>>>>> > > > > >>>>>>> > > > > >>>>> > > > > >> > > > > > > > > > > > > > >
