Re: Ignite-spring-boot-autoconfigurer
Hi Nikolay,
Thank you for your email. As part of Ignite Extensions migration we are
migrating Ignite Extensions to following repo.
https://github.com/apache/ignite-extensions
We have added flink and pub-sub modules and few additional modules are open
in PR.
You can refer to this PR to see how we are migrating the modules
https://github.com/apache/ignite-extensions/pull/5
I wanted to connect and discuss the changes to understand the spring boot
auto configure feature. We currently have an ignite spring module that
allows resource injection capabilities and provides a parser for Spring
based xml configuration files. Can you please review and share if the
changes you are proposing can be added as part of Ignite spring module or
it make sense to make it a separate spring boot auto configure module.
https://github.com/apache/ignite/tree/master/modules/spring
Regards,
Saikat
On Fri, Jan 17, 2020 at 3:12 AM Николай Ижиков wrote:
> Tests added.
> Please, review.
>
> Saikat, can you help with this PR [1]?
>
> I think it should be added as a separate module as we do with the flink
> integration.
> Can you help me with it?
> Do we have some how-to for it?
>
> [1] https://github.com/apache/ignite/pull/7237
>
> > 16 янв. 2020 г., в 16:51, Николай Ижиков
> написал(а):
> >
> > Hello, Denis.
> >
> > Thanks, for the feedback.
> >
> > Alexey, it seems, PR is ready to be reviewed, but I need some time(a day
> or two) to write tests.
> > You can start with the core code review if you wish.
> >
> > Here are autoconfigurer requirements:
> >
> > 1. Start usage of Ignite with minimal(or zero) configuration.
> > 2. Configure Ignite configuration properties with the standard spring
> boot application properties.
> > 3. Configure Ignite SPI implementation and so on that can’t be
> configured via #2.
> >
> > After some consultation with the Spring experts from the community(Maxim
> Stepachev thanks for the idea)
> > I updated the PR with the logic described below:
> >
> > 1. To enable Ignite auto-configuration user should add
> `org.apache.ignite:spring-boot-ignite-autoconfigure:2.9.0` to dependencies.
> >After it Ignite node will be started during spring-boot application
> startup.
> >
> > 2. IgniteConfiguration initialization logic:
> >
> > 2.1 If {@link IgniteConfiguration} bean exists in {@link BeanFactory} it
> will be used for the node start.
> > 2.2 If {@link IgniteConfiguration} bean doesn't exist following rules
> are applied:
> > * Newly introducer IgniteConfigurer bean will be used to customize an
> empty IgniteConfiguration instance.
> >If a user wants to set custom SPI instances or similar hardcoded
> values one should do it IgniteConfigurer implementation.
> >
> > * Application properties will override config values. Prefix for
> properties names is "ignite».
> >
> > PS. Similar logic applied for the second module -
> `org.apache.ignite:spring-boot-ignite-client-autoconfigure:2.9.0`.
> > It provides the same features but for the autoconfiguration of the
> IgniteClient
> >
> >
> >> 15 янв. 2020 г., в 03:03, Denis Magda написал(а):
> >>
> >> Nikolay,
> >>
> >> Thanks for contributing in this direction! That's one of the gaps on our
> >> end and the user community will be certainly thankful once we fill it
> in.
> >>
> >> *Alexey Kuznetsov*, as one of the Spring Boot experts, could you spend
> some
> >> time reviewing the changes?
> >>
> >> As for the extensions/modularization activities, please join Saikat in
> the
> >> discussions ([1] and [2]). He is contributing the foundation and moving
> our
> >> existing integrations to that new repository. The Spring Boot
> improvements
> >> might be moved or, another option, we might add this class to the core?
> >>
> >> [1]
> >>
> http://apache-ignite-developers.2346864.n4.nabble.com/IGNITE-12361-Migrate-Flume-module-to-ignite-extensions-td45010.html
> >> [2]
> >>
> http://apache-ignite-developers.2346864.n4.nabble.com/DISCUSS-Proposal-for-Ignite-Extensions-as-a-separate-Bahir-module-or-Incubator-project-td44064.html
> >>
> >> -
> >> Denis
> >>
> >>
> >> On Sat, Jan 11, 2020 at 10:44 AM Николай Ижиков
> wrote:
> >>
> >>> Hello, Igniters.
> >>>
> >>> During Ignite meetup I took part in there was a request from the users.
> >>> They propose to create a custom spring boot autoconfigurer module for
> >>> Ignite.
> >>> This module should provide a smooth injection of Ignite to any
> spring-boot
> >>> application.
> >>>
> >>> I've implemented a tiny straightforward prototype of the module [1]
> >>> Examples of the usage of integration can be found in the example
> >>> application [2]
> >>>
> >>> For now, the module provides the following features:
> >>>
> >>> 1. Starts Ignite node and inject it in the spring ApplicationContext if
> >>> bean of the type IgniteConfiguration exists in the context.
> >>> This can be achieved in two ways:
> >>> * create `IgniteConfiguration` from java code [3]
> >>> * add `ignite.xml` file to the application
Re: IGNITE-12361 Migrate Flume module to ignite-extensions
Hello Alexey, Thank you for your email. 1. Yes, we discussed in dev list and agreed on creating a new repository for hosting our Ignite integrations. Please find the discussion thread below. I will update the wiki page as well and share updates. http://apache-ignite-developers.2346864.n4.nabble.com/DISCUSS-Proposal-for-Ignite-Extensions-as-a-separate-Bahir-module-or-Incubator-project-td44064.html 2. I was hoping to complete migration of the following modules before we go ahead with release. I am tracking the jira story here https://issues.apache.org/jira/browse/IGNITE-12355 - Flink - Twitter - Storm - ZeroMQ - RocketMQ - Flume - MQTT - Camel - JMS 3. The dependencies for modules are pointing to latest snapshot of ignite project and if there are changes in ignite master branch then related affected Ignite extensions module also need to be modified. We will verify all the extensions for upcoming release but release only the one that are impacted. We will plan to avoid publishing any extension unless there are changes. Here is the discussion thread on release process: http://apache-ignite-developers.2346864.n4.nabble.com/DISCUSS-dependencies-and-release-process-for-Ignite-Extensions-td44478.html 4. Sounds good, we can maintain a compatibility matrix to ensure we can share Ignite Core and related Ignite Extensions. I have seen something similar in Apache Beam and we can also create a similar wiki page to maintain Ignite Core and Ignite Extensions dependencies https://beam.apache.org/documentation/runners/capability-matrix/ Regards, Saikat On Fri, Jan 17, 2020 at 6:27 AM Alexey Goncharuk wrote: > Saikat, > > Code-wise the PR looks ok because it basically moves the module to another > repo. I have some infrastructure/process questions though before we merge > the PRs > * I see that there are some objections from Alexey Zinoviev [1] on whether > the streaming modules should be placed in extensions or in a separate > streaming project. Sorry if I missed the corresponding discussion on the > dev-list. If there is one, we need to edit the IEP page, if not, I think we > should come to a consensus before making this change > * When are we planning to release the extensions? Are we going to release > them one-by-one incrementally or do we want to release them in one batch? > * Now that the release lifecycle of Ignite and extensions is split, we > need to understand how this is going to be tested. Will we run a candidate > Ignite build against already released extensions? What if we make changes > to both Ignite and an extension? Is it even allowed, or do we need to wait > for an Ignite release before making any changes in an extension? > * I think there should be some sort of compatibility matrix now that > releases of Ignite and extensions are separated > > I would not delete the extension from Ignite codebase until we agree on the > points above. > > Would you mind taking care of the discussion about it and formalizing it on > the wiki? > > чт, 16 янв. 2020 г. в 06:27, Saikat Maitra : > > > Hi Denis, Alexey > > > > Thank you so much for your email. I really appreciate it. > > > > Regards, > > Saikat > > > > On Wed, Jan 15, 2020 at 4:11 AM Alexey Goncharuk < > > alexey.goncha...@gmail.com> > > wrote: > > > > > Saikat, > > > > > > Thanks for working on this! I'll do my best to take a look at it this > > week. > > > > > > --AG > > > > > > пн, 13 янв. 2020 г. в 21:37, Denis Magda : > > > > > > > Alex Goncharuk, Nikolay Izhikov, > > > > > > > > Could you possibly check the changes or suggest any other > > > > committer/contributor for that? > > > > > > > > - > > > > Denis > > > > > > > > > > > > On Sun, Jan 12, 2020 at 9:31 AM Saikat Maitra < > saikat.mai...@gmail.com > > > > > > > wrote: > > > > > > > >> Hello, > > > >> > > > >> These PRs are part of Modularization effort > > > >> > > > >> > > > > > > https://cwiki.apache.org/confluence/display/IGNITE/IEP-36%3A+Modularization#IEP-36:Modularization-IndependentIntegrations > > > >> > > > >> If the changes looks good, I can go ahead and merge the changes. > > > >> > > > >> Regards, > > > >> Saikat > > > >> > > > >> On Sun, Jan 5, 2020 at 1:08 PM Saikat Maitra < > saikat.mai...@gmail.com > > > > > > >> wrote: > > > >> > > > >> > Hi, > > > >> > > > > >> > I have raised PR for Ignite Flume migration to Ignite Extensions > > repo. > > > >> > > > > >> > Jira https://issues.apache.org/jira/browse/IGNITE-12361 > > > >> > > > > >> > PR https://github.com/apache/ignite-extensions/pull/4 > > > >> > https://github.com/apache/ignite/pull/7227 > > > >> > > > > >> > Please review and share feedback. > > > >> > > > > >> > This is part of our Modularization effort for Streamer modules > > > >> > > > > >> > https://issues.apache.org/jira/browse/IGNITE-12355 > > > >> > > > > >> > Regards, > > > >> > Saikat > > > >> > > > > >> > > > > > > > > > >
Re: IGNITE-12358 Migrate ZeroMQ module to ignite-extensions
Yes sure, thank you Denis Regards, Saikat On Thu, Jan 16, 2020 at 3:53 PM Denis Magda wrote: > Hi Saikat, > > Let's wait while Alex Goncharuk checks a similar PR here: > > http://apache-ignite-developers.2346864.n4.nabble.com/IGNITE-12361-Migrate-Flume-module-to-ignite-extensions-td45010.html > > After that, we can return to this pending pull request. > > - > Denis > > > On Sun, Jan 12, 2020 at 12:59 PM Saikat Maitra > wrote: > > > Hi, > > > > I have raised PR for Ignite zeromq migration to Ignite Extensions repo. > > > > Jira https://issues.apache.org/jira/browse/IGNITE-12358 > > > > PR https://github.com/apache/ignite-extensions/pull/5 > > https://github.com/apache/ignite/pull/7240 > > > > Please review and share feedback. > > > > This is part of our Modularization effort for Streamer modules > > > > https://issues.apache.org/jira/browse/IGNITE-12355 > > > > Regards, > > Saikat > > >
Ignite to H20 integration
Have any of you performed an H20 integration with Ignite to import an extracted feature data set directly as input into Ignite training engine? -- Sent from: http://apache-ignite-developers.2346864.n4.nabble.com/
ML Model persist and reuse
Hello all Apache Ignite ML developers: I understand currently Ignite can't save a model after training, in such a way that the model can be re-imported by another Ignite cluster. Correct me if you can save and reload a model but I don't think you can. Anyway, I'd like to know if you have recommendations on how you can do either one of the following: 1. convert the Ignite model into an interchangeable format? For example there are some emerging standards (such as https://onnx.ai/ for one) and others - have any of you worked with such 2. if not transform the Ignite model into some standard format, how about saving the model into Native persistence, binary serialized format, creating some kind of handle that can shared with other clusters, and then use this to reload the model into a new Ignite session? This question has been asked of me recently, and this would be a good way to let Apache Ignite ML/DL models participate in a broader enterprise model deployment process. -- Sent from: http://apache-ignite-developers.2346864.n4.nabble.com/
Re: Internal classes are exposed in public API
+1 to mark feature or whole release as EA. пт, 17 янв. 2020 г., 23:00 Denis Magda : > 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 Николай Ижиков > 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 написал(а): > > > > > >>> 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 Николай Ижиков > > wrote: > > >> > > >> Andrey. > > >> > > >> All issues Alexey
Re: Internal classes are exposed in public API
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 Николай Ижиков 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 написал(а): > > > >>> 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 Николай Ижиков > 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
Re: [DISCUSSION] API to KILL any user started computation
Hello, Andrey. Thanks for positive feedback. Appreciate it. > we can't cancel service or service's method I understand it. Seems, we have several obvious options here: * Try to do it and if fail answer to the user: «I’ve tried and fail. Sorry» * Kill hanging thread. > This invocations not register anywhere and can't be tracked and killed. Yes. So we should invent tracking for those invocation to be able to kill it :) > 17 янв. 2020 г., в 21:03, Andrey Gura написал(а): > > It would be grate. > > Only one comment: we can't cancel service or service's method > execution because service grid API doesn't imply any requirement to > service implementation. So if user do something like infinite cycle or > blocking but non-interruptible operation it's impossible to interrupt > it. Also service method invocation itself isn't cluster wide or local > node specific operation. This invocations not register anywhere and > can't be tracked and killed. > > Unfortunately, the same is valid for some jobs in sense of infinite or > non-interruptible operations. > > On Wed, Jan 15, 2020 at 9:30 PM Николай Ижиков wrote: >> >> Hello, Igniters. >> >> As you may know, we put a lot of effort to improve Ignite metric and >> diagnostic API. >> We have created the following API: >>* Metric manager >>* System view manager >> As far as I know, we would have tracing capabilities soon. >> >> I think it's time to take the next step. >> We should provide to the administrator the API to cancel(stop, kill) >> arbitrary user started computation. >> >> Right now we have API to do it for: >>* transactions `TransactionsMXBean#getActiveTransactions`. >>* SQL queries: `KILL QUERY` sql command and visor task - >> `VisorQueryCancelTask` >> >> Please, note, these features are implemented via different API. >> >> I think we should introduce uniform Cancel API for the following >> computations: >> >>* service. >>* specific service method execution. >>* compute job(compute task). >>* query(scan, continuous, text). >> >> We should also rework kill transaction and kill SQL queries API to make >> them similar to each other. >> I have plans to write an IEP for it and implement it. >> What do you think? >>
Re: Internal classes are exposed in public API
> 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 написал(а): > >>> 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 Николай Ижиков 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 написал(а): >>> >>> 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
Re: [DISCUSSION] API to KILL any user started computation
It would be grate. Only one comment: we can't cancel service or service's method execution because service grid API doesn't imply any requirement to service implementation. So if user do something like infinite cycle or blocking but non-interruptible operation it's impossible to interrupt it. Also service method invocation itself isn't cluster wide or local node specific operation. This invocations not register anywhere and can't be tracked and killed. Unfortunately, the same is valid for some jobs in sense of infinite or non-interruptible operations. On Wed, Jan 15, 2020 at 9:30 PM Николай Ижиков wrote: > > Hello, Igniters. > > As you may know, we put a lot of effort to improve Ignite metric and > diagnostic API. > We have created the following API: > * Metric manager > * System view manager > As far as I know, we would have tracing capabilities soon. > > I think it's time to take the next step. > We should provide to the administrator the API to cancel(stop, kill) > arbitrary user started computation. > > Right now we have API to do it for: > * transactions `TransactionsMXBean#getActiveTransactions`. > * SQL queries: `KILL QUERY` sql command and visor task - > `VisorQueryCancelTask` > > Please, note, these features are implemented via different API. > > I think we should introduce uniform Cancel API for the following computations: > > * service. > * specific service method execution. > * compute job(compute task). > * query(scan, continuous, text). > > We should also rework kill transaction and kill SQL queries API to make them > similar to each other. > I have plans to write an IEP for it and implement it. > What do you think? >
Re: Internal classes are exposed in public API
>> 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 Николай Ижиков 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 написал(а): > > > > 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 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
Re: Internal classes are exposed in public API
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 написал(а): > > 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 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 Николай Ижиков 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 написал(а):
Re: Internal classes are exposed in public API
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 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 Николай Ижиков 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 написал(а): > > > > > >> 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
Re: Internal classes are exposed in public API
>> 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 Николай Ижиков 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 написал(а): > > > >> 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. > > > >
Re: Internal classes are exposed in public API
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 написал(а): > >> 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 Николай Ижиков 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
Re: Internal classes are exposed in public API
> 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 Николай Ижиков 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 написал(а): > > > > 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
Re: Apache Ignite 2.8 RELEASE [Time, Scope, Manager]
Maxim, I will do that on monday (20/01). пт, 17 янв. 2020 г. в 13:08, Maxim Muzafarov : > Sergey, > > > Can you, please, resolve the PR conflicts [1] [2]? > > [1] https://github.com/apache/ignite/pull/7238 > [2] https://issues.apache.org/jira/browse/IGNITE-11256 > > On Thu, 16 Jan 2020 at 16:59, Ilya Kasnacheev > wrote: > > > > Hello! > > > > I have bumped beanutils and re-ran Cassandra Store tests. Can you please > > comment on the ticket? > > > > I think that fixing ZooKeeper is too much effort (there's chaos with > > jackson vs. jackson-asl), maybe it should be split up as a separate > ticket > > to be done later. > > > > Regards, > > -- > > Ilya Kasnacheev > > > > > > ср, 15 янв. 2020 г. в 18:31, Vladimir Pligin : > > > > > Thanks, Ilya. It would be really great to have your patch included > into 2.8 > > > scope. > > > I'd like to give my two cent as well. For example we have vulnerable > > > dependencies here: > > > modules/cassandra/store/pom.xml - commons-beanutils > > > modules/zookeeper/pom.xml - transitive Jackson from Curator > > > > > > I'd suggest to uprgrade commons-beanutils:commons-beanutils to 1.9.4 > and > > > override com.fasterxml.jackson.core:jackson-databind to our common > jackson > > > version from other modules. > > > > > > > > > > > > -- > > > Sent from: http://apache-ignite-developers.2346864.n4.nabble.com/ > > > > -- BR, Sergey Antonov
Re: Internal classes are exposed in public API
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 написал(а): > > 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 Николай Ижиков 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 >>> написал(а): >>> >>> 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 >>>
Re: IGNITE-12361 Migrate Flume module to ignite-extensions
Saikat, Code-wise the PR looks ok because it basically moves the module to another repo. I have some infrastructure/process questions though before we merge the PRs * I see that there are some objections from Alexey Zinoviev [1] on whether the streaming modules should be placed in extensions or in a separate streaming project. Sorry if I missed the corresponding discussion on the dev-list. If there is one, we need to edit the IEP page, if not, I think we should come to a consensus before making this change * When are we planning to release the extensions? Are we going to release them one-by-one incrementally or do we want to release them in one batch? * Now that the release lifecycle of Ignite and extensions is split, we need to understand how this is going to be tested. Will we run a candidate Ignite build against already released extensions? What if we make changes to both Ignite and an extension? Is it even allowed, or do we need to wait for an Ignite release before making any changes in an extension? * I think there should be some sort of compatibility matrix now that releases of Ignite and extensions are separated I would not delete the extension from Ignite codebase until we agree on the points above. Would you mind taking care of the discussion about it and formalizing it on the wiki? чт, 16 янв. 2020 г. в 06:27, Saikat Maitra : > Hi Denis, Alexey > > Thank you so much for your email. I really appreciate it. > > Regards, > Saikat > > On Wed, Jan 15, 2020 at 4:11 AM Alexey Goncharuk < > alexey.goncha...@gmail.com> > wrote: > > > Saikat, > > > > Thanks for working on this! I'll do my best to take a look at it this > week. > > > > --AG > > > > пн, 13 янв. 2020 г. в 21:37, Denis Magda : > > > > > Alex Goncharuk, Nikolay Izhikov, > > > > > > Could you possibly check the changes or suggest any other > > > committer/contributor for that? > > > > > > - > > > Denis > > > > > > > > > On Sun, Jan 12, 2020 at 9:31 AM Saikat Maitra > > > > wrote: > > > > > >> Hello, > > >> > > >> These PRs are part of Modularization effort > > >> > > >> > > > https://cwiki.apache.org/confluence/display/IGNITE/IEP-36%3A+Modularization#IEP-36:Modularization-IndependentIntegrations > > >> > > >> If the changes looks good, I can go ahead and merge the changes. > > >> > > >> Regards, > > >> Saikat > > >> > > >> On Sun, Jan 5, 2020 at 1:08 PM Saikat Maitra > > > >> wrote: > > >> > > >> > Hi, > > >> > > > >> > I have raised PR for Ignite Flume migration to Ignite Extensions > repo. > > >> > > > >> > Jira https://issues.apache.org/jira/browse/IGNITE-12361 > > >> > > > >> > PR https://github.com/apache/ignite-extensions/pull/4 > > >> > https://github.com/apache/ignite/pull/7227 > > >> > > > >> > Please review and share feedback. > > >> > > > >> > This is part of our Modularization effort for Streamer modules > > >> > > > >> > https://issues.apache.org/jira/browse/IGNITE-12355 > > >> > > > >> > Regards, > > >> > Saikat > > >> > > > >> > > > > > >
Re: Internal classes are exposed in public API
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 Николай Ижиков 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 > > написал(а): > > > > 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 >
Re: [DISCUSSION] API to KILL any user started computation
Hi Alexei, > Not sure I've understand a question. JMX supports security via password protection and secure channel. That is the obvious thing I missed! :) Thank you! Thanks, S. пт, 17 янв. 2020 г. в 14:45, Alexei Scherbakov : > Slava, > > Not sure I've understand a question. JMX supports security via password > protection and secure channel. > > пт, 17 янв. 2020 г. в 14:30, Вячеслав Коптилин : > > > Hello Nikolay, > > > > I'm not sure we need JMX API for this case. If I’m not mistaken, it’s > about > > the administrator’s ability to cancel any task/query in the cluster, and > in > > my opinion, such action must require administrator permission. > > Could you please clarify how it can be done via JMX? I mean permission > > check and etc. Perhaps, I'm missing something obvious... > > > > Thanks, > > S. > > > > чт, 16 янв. 2020 г. в 15:46, Николай Ижиков : > > > > > Alexey. > > > > > > I think, yes. > > > We certainly should be able to use system view data for the new KILL > API. > > > > > > I think we should support both SQL and Java(JMX) API for this KILL > > command. > > > > > > > > > > 16 янв. 2020 г., в 15:28, Alexei Scherbakov < > > > alexey.scherbak...@gmail.com> написал(а): > > > > > > > > Nikolaj, > > > > > > > > Can we use system views instead of implementing something new ? > > > > > > > > Each user operation has an unique ID. > > > > > > > > It's possible to introduce universal SQL kill something like: > > > > > > > > kill transaction ID > > > > > > > > where id is taken from system view. > > > > > > > > > > > > чт, 16 янв. 2020 г. в 14:19, Николай Ижиков : > > > > > > > >> Hello, Alexey. > > > >> > > > >> I’m talking about *administrator* API. > > > >> > > > >> For example, the User has a cluster that is used by several > > > applications. > > > >> Some application starts buggy compute tasks that consume all CPU > > > resources. > > > >> Right now, administrator doesn’t have the ability to kill this task. > > > >> > > > >> This can lead to instability of the whole cluster. > > > >> > > > >> > > > >>> 16 янв. 2020 г., в 13:42, Alexei Scherbakov < > > > >> alexey.scherbak...@gmail.com> написал(а): > > > >>> > > > >>> Transactions can be also rolled back using tx.close where close is > > > >>> java.lang.AutoCloseable#close > > > >>> It looks for me to the definition of uniform cancel API. > > > >>> > > > >>> > > > >>> > > > >>> чт, 16 янв. 2020 г. в 13:37, Alexei Scherbakov < > > > >> alexey.scherbak...@gmail.com > > > : > > > >>> > > > Nikolaj, > > > > > > We already have cancellation possibilities for almost every user > > > computation. > > > Transactions are cancelled using tx.rollback() > > > Queries are cancelled using query.close() > > > Task is cancellable through ComputeTaskSession > > > > > > What is uniform cancel API ? Why do we need it ? > > > > > > > > > > > > ср, 15 янв. 2020 г. в 21:30, Николай Ижиков >: > > > > > > > Hello, Igniters. > > > > > > > > As you may know, we put a lot of effort to improve Ignite metric > > and > > > > diagnostic API. > > > > We have created the following API: > > > > * Metric manager > > > > * System view manager > > > > As far as I know, we would have tracing capabilities soon. > > > > > > > > I think it's time to take the next step. > > > > We should provide to the administrator the API to cancel(stop, > > kill) > > > > arbitrary user started computation. > > > > > > > > Right now we have API to do it for: > > > > * transactions `TransactionsMXBean#getActiveTransactions`. > > > > * SQL queries: `KILL QUERY` sql command and visor task - > > > > `VisorQueryCancelTask` > > > > > > > > Please, note, these features are implemented via different API. > > > > > > > > I think we should introduce uniform Cancel API for the following > > > > computations: > > > > > > > > * service. > > > > * specific service method execution. > > > > * compute job(compute task). > > > > * query(scan, continuous, text). > > > > > > > > We should also rework kill transaction and kill SQL queries API > to > > > >> make > > > > them similar to each other. > > > > I have plans to write an IEP for it and implement it. > > > > What do you think? > > > > > > > > > > > > > > -- > > > > > > Best regards, > > > Alexei Scherbakov > > > > > > >>> > > > >>> > > > >>> -- > > > >>> > > > >>> Best regards, > > > >>> Alexei Scherbakov > > > >> > > > >> > > > > > > > > -- > > > > > > > > Best regards, > > > > Alexei Scherbakov > > > > > > > > > > > -- > > Best regards, > Alexei Scherbakov >
Re: [DISCUSSION] API to KILL any user started computation
Slava, Not sure I've understand a question. JMX supports security via password protection and secure channel. пт, 17 янв. 2020 г. в 14:30, Вячеслав Коптилин : > Hello Nikolay, > > I'm not sure we need JMX API for this case. If I’m not mistaken, it’s about > the administrator’s ability to cancel any task/query in the cluster, and in > my opinion, such action must require administrator permission. > Could you please clarify how it can be done via JMX? I mean permission > check and etc. Perhaps, I'm missing something obvious... > > Thanks, > S. > > чт, 16 янв. 2020 г. в 15:46, Николай Ижиков : > > > Alexey. > > > > I think, yes. > > We certainly should be able to use system view data for the new KILL API. > > > > I think we should support both SQL and Java(JMX) API for this KILL > command. > > > > > > > 16 янв. 2020 г., в 15:28, Alexei Scherbakov < > > alexey.scherbak...@gmail.com> написал(а): > > > > > > Nikolaj, > > > > > > Can we use system views instead of implementing something new ? > > > > > > Each user operation has an unique ID. > > > > > > It's possible to introduce universal SQL kill something like: > > > > > > kill transaction ID > > > > > > where id is taken from system view. > > > > > > > > > чт, 16 янв. 2020 г. в 14:19, Николай Ижиков : > > > > > >> Hello, Alexey. > > >> > > >> I’m talking about *administrator* API. > > >> > > >> For example, the User has a cluster that is used by several > > applications. > > >> Some application starts buggy compute tasks that consume all CPU > > resources. > > >> Right now, administrator doesn’t have the ability to kill this task. > > >> > > >> This can lead to instability of the whole cluster. > > >> > > >> > > >>> 16 янв. 2020 г., в 13:42, Alexei Scherbakov < > > >> alexey.scherbak...@gmail.com> написал(а): > > >>> > > >>> Transactions can be also rolled back using tx.close where close is > > >>> java.lang.AutoCloseable#close > > >>> It looks for me to the definition of uniform cancel API. > > >>> > > >>> > > >>> > > >>> чт, 16 янв. 2020 г. в 13:37, Alexei Scherbakov < > > >> alexey.scherbak...@gmail.com > > : > > >>> > > Nikolaj, > > > > We already have cancellation possibilities for almost every user > > computation. > > Transactions are cancelled using tx.rollback() > > Queries are cancelled using query.close() > > Task is cancellable through ComputeTaskSession > > > > What is uniform cancel API ? Why do we need it ? > > > > > > > > ср, 15 янв. 2020 г. в 21:30, Николай Ижиков : > > > > > Hello, Igniters. > > > > > > As you may know, we put a lot of effort to improve Ignite metric > and > > > diagnostic API. > > > We have created the following API: > > > * Metric manager > > > * System view manager > > > As far as I know, we would have tracing capabilities soon. > > > > > > I think it's time to take the next step. > > > We should provide to the administrator the API to cancel(stop, > kill) > > > arbitrary user started computation. > > > > > > Right now we have API to do it for: > > > * transactions `TransactionsMXBean#getActiveTransactions`. > > > * SQL queries: `KILL QUERY` sql command and visor task - > > > `VisorQueryCancelTask` > > > > > > Please, note, these features are implemented via different API. > > > > > > I think we should introduce uniform Cancel API for the following > > > computations: > > > > > > * service. > > > * specific service method execution. > > > * compute job(compute task). > > > * query(scan, continuous, text). > > > > > > We should also rework kill transaction and kill SQL queries API to > > >> make > > > them similar to each other. > > > I have plans to write an IEP for it and implement it. > > > What do you think? > > > > > > > > > > -- > > > > Best regards, > > Alexei Scherbakov > > > > >>> > > >>> > > >>> -- > > >>> > > >>> Best regards, > > >>> Alexei Scherbakov > > >> > > >> > > > > > > -- > > > > > > Best regards, > > > Alexei Scherbakov > > > > > -- Best regards, Alexei Scherbakov
Re: Internal classes are exposed in public API
Nikolay, thanks! I am not insisting we must implement IGNITE-12553 right away nor am I saying it's mandatory. I just want to make sure we do not deprecate existing APIs unless we as a community are certain that they will be dropped and there is no need for the replacement. Will take a look at the PR shortly. пт, 17 янв. 2020 г. в 12:49, Николай Ижиков : > 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 > написал(а): > > > > 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 > >
Re: [DISCUSSION] API to KILL any user started computation
Hello Nikolay, I'm not sure we need JMX API for this case. If I’m not mistaken, it’s about the administrator’s ability to cancel any task/query in the cluster, and in my opinion, such action must require administrator permission. Could you please clarify how it can be done via JMX? I mean permission check and etc. Perhaps, I'm missing something obvious... Thanks, S. чт, 16 янв. 2020 г. в 15:46, Николай Ижиков : > Alexey. > > I think, yes. > We certainly should be able to use system view data for the new KILL API. > > I think we should support both SQL and Java(JMX) API for this KILL command. > > > > 16 янв. 2020 г., в 15:28, Alexei Scherbakov < > alexey.scherbak...@gmail.com> написал(а): > > > > Nikolaj, > > > > Can we use system views instead of implementing something new ? > > > > Each user operation has an unique ID. > > > > It's possible to introduce universal SQL kill something like: > > > > kill transaction ID > > > > where id is taken from system view. > > > > > > чт, 16 янв. 2020 г. в 14:19, Николай Ижиков : > > > >> Hello, Alexey. > >> > >> I’m talking about *administrator* API. > >> > >> For example, the User has a cluster that is used by several > applications. > >> Some application starts buggy compute tasks that consume all CPU > resources. > >> Right now, administrator doesn’t have the ability to kill this task. > >> > >> This can lead to instability of the whole cluster. > >> > >> > >>> 16 янв. 2020 г., в 13:42, Alexei Scherbakov < > >> alexey.scherbak...@gmail.com> написал(а): > >>> > >>> Transactions can be also rolled back using tx.close where close is > >>> java.lang.AutoCloseable#close > >>> It looks for me to the definition of uniform cancel API. > >>> > >>> > >>> > >>> чт, 16 янв. 2020 г. в 13:37, Alexei Scherbakov < > >> alexey.scherbak...@gmail.com > : > >>> > Nikolaj, > > We already have cancellation possibilities for almost every user > computation. > Transactions are cancelled using tx.rollback() > Queries are cancelled using query.close() > Task is cancellable through ComputeTaskSession > > What is uniform cancel API ? Why do we need it ? > > > > ср, 15 янв. 2020 г. в 21:30, Николай Ижиков : > > > Hello, Igniters. > > > > As you may know, we put a lot of effort to improve Ignite metric and > > diagnostic API. > > We have created the following API: > > * Metric manager > > * System view manager > > As far as I know, we would have tracing capabilities soon. > > > > I think it's time to take the next step. > > We should provide to the administrator the API to cancel(stop, kill) > > arbitrary user started computation. > > > > Right now we have API to do it for: > > * transactions `TransactionsMXBean#getActiveTransactions`. > > * SQL queries: `KILL QUERY` sql command and visor task - > > `VisorQueryCancelTask` > > > > Please, note, these features are implemented via different API. > > > > I think we should introduce uniform Cancel API for the following > > computations: > > > > * service. > > * specific service method execution. > > * compute job(compute task). > > * query(scan, continuous, text). > > > > We should also rework kill transaction and kill SQL queries API to > >> make > > them similar to each other. > > I have plans to write an IEP for it and implement it. > > What do you think? > > > > > > -- > > Best regards, > Alexei Scherbakov > > >>> > >>> > >>> -- > >>> > >>> Best regards, > >>> Alexei Scherbakov > >> > >> > > > > -- > > > > Best regards, > > Alexei Scherbakov > >
Re: Apache Ignite 2.8 RELEASE [Time, Scope, Manager]
Sergey, Can you, please, resolve the PR conflicts [1] [2]? [1] https://github.com/apache/ignite/pull/7238 [2] https://issues.apache.org/jira/browse/IGNITE-11256 On Thu, 16 Jan 2020 at 16:59, Ilya Kasnacheev wrote: > > Hello! > > I have bumped beanutils and re-ran Cassandra Store tests. Can you please > comment on the ticket? > > I think that fixing ZooKeeper is too much effort (there's chaos with > jackson vs. jackson-asl), maybe it should be split up as a separate ticket > to be done later. > > Regards, > -- > Ilya Kasnacheev > > > ср, 15 янв. 2020 г. в 18:31, Vladimir Pligin : > > > Thanks, Ilya. It would be really great to have your patch included into 2.8 > > scope. > > I'd like to give my two cent as well. For example we have vulnerable > > dependencies here: > > modules/cassandra/store/pom.xml - commons-beanutils > > modules/zookeeper/pom.xml - transitive Jackson from Curator > > > > I'd suggest to uprgrade commons-beanutils:commons-beanutils to 1.9.4 and > > override com.fasterxml.jackson.core:jackson-databind to our common jackson > > version from other modules. > > > > > > > > -- > > Sent from: http://apache-ignite-developers.2346864.n4.nabble.com/ > >
Re: Internal classes are exposed in public API
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 > написал(а): > > 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
[jira] [Created] (IGNITE-12553) [IEP-35] public Java metric API
Nikolay Izhikov created IGNITE-12553: Summary: [IEP-35] public Java metric API Key: IGNITE-12553 URL: https://issues.apache.org/jira/browse/IGNITE-12553 Project: Ignite Issue Type: Improvement Reporter: Nikolay Izhikov Right now, there is no simple way to get metrics values from the java code. We need to create a java API to access metric values. -- This message was sent by Atlassian Jira (v8.3.4#803005)
Re: Ignite-spring-boot-autoconfigurer
Tests added.
Please, review.
Saikat, can you help with this PR [1]?
I think it should be added as a separate module as we do with the flink
integration.
Can you help me with it?
Do we have some how-to for it?
[1] https://github.com/apache/ignite/pull/7237
> 16 янв. 2020 г., в 16:51, Николай Ижиков написал(а):
>
> Hello, Denis.
>
> Thanks, for the feedback.
>
> Alexey, it seems, PR is ready to be reviewed, but I need some time(a day or
> two) to write tests.
> You can start with the core code review if you wish.
>
> Here are autoconfigurer requirements:
>
> 1. Start usage of Ignite with minimal(or zero) configuration.
> 2. Configure Ignite configuration properties with the standard spring boot
> application properties.
> 3. Configure Ignite SPI implementation and so on that can’t be configured via
> #2.
>
> After some consultation with the Spring experts from the community(Maxim
> Stepachev thanks for the idea)
> I updated the PR with the logic described below:
>
> 1. To enable Ignite auto-configuration user should add
> `org.apache.ignite:spring-boot-ignite-autoconfigure:2.9.0` to dependencies.
>After it Ignite node will be started during spring-boot application
> startup.
>
> 2. IgniteConfiguration initialization logic:
>
> 2.1 If {@link IgniteConfiguration} bean exists in {@link BeanFactory} it will
> be used for the node start.
> 2.2 If {@link IgniteConfiguration} bean doesn't exist following rules are
> applied:
> * Newly introducer IgniteConfigurer bean will be used to customize an empty
> IgniteConfiguration instance.
>If a user wants to set custom SPI instances or similar hardcoded values
> one should do it IgniteConfigurer implementation.
>
> * Application properties will override config values. Prefix for properties
> names is "ignite».
>
> PS. Similar logic applied for the second module -
> `org.apache.ignite:spring-boot-ignite-client-autoconfigure:2.9.0`.
> It provides the same features but for the autoconfiguration of the
> IgniteClient
>
>
>> 15 янв. 2020 г., в 03:03, Denis Magda написал(а):
>>
>> Nikolay,
>>
>> Thanks for contributing in this direction! That's one of the gaps on our
>> end and the user community will be certainly thankful once we fill it in.
>>
>> *Alexey Kuznetsov*, as one of the Spring Boot experts, could you spend some
>> time reviewing the changes?
>>
>> As for the extensions/modularization activities, please join Saikat in the
>> discussions ([1] and [2]). He is contributing the foundation and moving our
>> existing integrations to that new repository. The Spring Boot improvements
>> might be moved or, another option, we might add this class to the core?
>>
>> [1]
>> http://apache-ignite-developers.2346864.n4.nabble.com/IGNITE-12361-Migrate-Flume-module-to-ignite-extensions-td45010.html
>> [2]
>> http://apache-ignite-developers.2346864.n4.nabble.com/DISCUSS-Proposal-for-Ignite-Extensions-as-a-separate-Bahir-module-or-Incubator-project-td44064.html
>>
>> -
>> Denis
>>
>>
>> On Sat, Jan 11, 2020 at 10:44 AM Николай Ижиков wrote:
>>
>>> Hello, Igniters.
>>>
>>> During Ignite meetup I took part in there was a request from the users.
>>> They propose to create a custom spring boot autoconfigurer module for
>>> Ignite.
>>> This module should provide a smooth injection of Ignite to any spring-boot
>>> application.
>>>
>>> I've implemented a tiny straightforward prototype of the module [1]
>>> Examples of the usage of integration can be found in the example
>>> application [2]
>>>
>>> For now, the module provides the following features:
>>>
>>> 1. Starts Ignite node and inject it in the spring ApplicationContext if
>>> bean of the type IgniteConfiguration exists in the context.
>>> This can be achieved in two ways:
>>> * create `IgniteConfiguration` from java code [3]
>>> * add `ignite.xml` file to the application context [4]
>>>
>>> 2. Starts IgniteClient instance and injects it int the spring Application
>>> if:
>>> * ClientConfiguration bean exists in the context [5]
>>> * `spring.data.ignite.clientAddresses` exists in the application
>>> properties. [6]
>>>
>>> I have a following questions regards new module:
>>>
>>> 1. We have an extension initiative so where is the right place for the
>>> new module?
>>> 2. Do we have spring experts in the community? What other features for
>>> this autoconfigurer module required?
>>>
>>> [1] https://github.com/apache/ignite/pull/7237/files
>>> [2] https://github.com/nizhikov/ignite-spring-boot-autoconfigure-example
>>> [3]
>>> https://github.com/nizhikov/ignite-spring-boot-autoconfigure-example/tree/master/src/main/java/org/apache/ignite/spring/boot/configfrombean
>>> [4]
>>> https://github.com/nizhikov/ignite-spring-boot-autoconfigure-example/tree/master/src/main/java/org/apache/ignite/spring/boot/configfromfile
>>> [5]
>>>
Re: Internal classes are exposed in public API
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
