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