Hi Saikat, Thank you for driving it. I left my comments [1].
[1] https://issues.apache.org/jira/browse/IGNITE-7285 сб, 15 июн. 2019 г. в 20:48, Saikat Maitra <saikat.mai...@gmail.com>: > > Hi Ivan, > > Thank you for your email. I have updated the PR to use default query > timeout. > > Please take a look. > > https://github.com/apache/ignite/pull/6490/files > > Regards > Saikat > > On Tue, May 7, 2019 at 12:30 AM Ivan Pavlukhina <vololo...@gmail.com> wrote: > > > Hi Saikat, > > > > It is good that we agreed how property in question should be configured. > > But I worry about the following. If the PR merged it will not contain a > > user value yet because an introduced property is not used. Consequently we > > must start using that property before a next AI release. Just one thing to > > keep in mind. > > > > Sent from my iPhone > > > > > On 4 May 2019, at 05:56, Saikat Maitra <saikat.mai...@gmail.com> wrote: > > > > > > Hi Ivan, > > > > > > Thank you for reviewing the PR. I have updated the PR. Please review and > > > share your feedback. > > > > > > I was thinking of making a separate PR for using the defaultQueryTimeout > > > property in query execution flow. > > > > > > Regards, > > > Saikat > > > > > >> On Wed, May 1, 2019 at 1:04 AM Павлухин Иван <vololo...@gmail.com> > > wrote: > > >> > > >> Andrey K., > > >> > > >>> I think we should develop some kind of "Queries" options on Ignite > > >>> configuration. > > >> > > >> Quite a reasonable idea. We already have couple of query-related > > >> properties in IgniteConfiguration and we can move them (in a > > >> compatible way) to a query properties sub-aggregate. I think it is > > >> better to raise a separate topic for that. > > >> > > >> ср, 1 мая 2019 г. в 09:00, Павлухин Иван <vololo...@gmail.com>: > > >>> > > >>> Hi Saikat, > > >>> > > >>> I left a couple of comment on GitHub [1]. > > >>> > > >>> Perhaps I am missing it but what are the plans for making a default > > >>> query timeout workable by using an introduced property in query > > >>> execution flow? > > >>> > > >>> [1] https://github.com/apache/ignite/pull/6490 > > >>> > > >>> вт, 30 апр. 2019 г. в 02:50, Saikat Maitra <saikat.mai...@gmail.com>: > > >>>> > > >>>> Hi Ivan, > > >>>> > > >>>> Yes, I checked this CacheQuery default value > > >>>> > > >> > > https://github.com/apache/ignite/blob/master/modules/core/src/main/java/org/apache/ignite/internal/processors/cache/query/CacheQuery.java#L200 > > >>>> > > >>>> Also, Andrew recommended the same in review feedback. > > >>>> > > >>>> https://github.com/apache/ignite/pull/6490#discussion_r277730394 > > >>>> > > >>>> Regards, > > >>>> Saikat > > >>>> > > >>>> > > >>>> On Mon, Apr 29, 2019 at 3:18 AM Павлухин Иван <vololo...@gmail.com> > > >> wrote: > > >>>> > > >>>>> Hi Saikat, > > >>>>> > > >>>>> It a compatibility with previous versions the reason for an > > >> indefinite > > >>>>> timeout by default? > > >>>>> > > >>>>> сб, 27 апр. 2019 г. в 16:58, Saikat Maitra <saikat.mai...@gmail.com > > >>> : > > >>>>>> > > >>>>>> Hi Alexey, Ivan, Andrew > > >>>>>> > > >>>>>> I think we can provide an option to configure defaultQueryOption at > > >>>>>> IgniteConfiguration and set the default value = 0 to imply if not > > >> set it > > >>>>>> will be execute indefinitely but then user can set this value > > >> based on > > >>>>> the > > >>>>>> application preferences and use it in addition to SQL query > > >> timeout. > > >>>>>> > > >>>>>> I have updated the PR accordingly. > > >>>>>> > > >>>>>> Please review and share if any changes required. > > >>>>>> > > >>>>>> Regards, > > >>>>>> Saikat > > >>>>>> > > >>>>>> On Wed, Apr 24, 2019 at 4:33 AM Alexey Kuznetsov < > > >> akuznet...@apache.org> > > >>>>>> wrote: > > >>>>>> > > >>>>>>> Hi Saikat and Ivan, > > >>>>>>> > > >>>>>>> I think that properties that related to SQL should not be > > >> configured on > > >>>>>>> caches. > > >>>>>>> We already put a lot of effort to decouple SQL from caches. > > >>>>>>> > > >>>>>>> I think we should develop some kind of "Queries" options on > > >> Ignite > > >>>>>>> configuration. > > >>>>>>> > > >>>>>>> What do you think? > > >>>>>>> > > >>>>>>> > > >>>>>>> On Wed, Apr 24, 2019 at 3:22 PM Павлухин Иван < > > >> vololo...@gmail.com> > > >>>>> wrote: > > >>>>>>> > > >>>>>>>> Hi Saikat, > > >>>>>>>> > > >>>>>>>> I think that we should have a discussion and choose a place > > >> where a > > >>>>>>>> "default query timeout" property will be configured. > > >>>>>>>> > > >>>>>>>> Generally, I think that a real (user) problem is possibility > > >> for > > >>>>>>>> queries to execute indefinitely. And I have no doubts that we > > >> can > > >>>>>>>> improve there. There could be several implementation > > >> strategies. One > > >>>>>>>> is adding a property to CacheConfiguration. But it opens > > >> various > > >>>>>>>> questions. E.g. how should it work if we execute SQL JOIN > > >> spanning > > >>>>>>>> multiple tables (caches)? Also I am concerned about queries > > >> executed > > >>>>>>>> not via cache.query() method. We have multiple alternative > > >> options, > > >>>>>>>> e.g. thin clients (IgniteClient.query) or JDBC. I believe that > > >>>>>>>> introducing a default timeout for all them is not a bad idea. > > >>>>>>>> > > >>>>>>>> What do you think? > > >>>>>>>> > > >>>>>>>> вт, 23 апр. 2019 г. в 03:01, Saikat Maitra < > > >> saikat.mai...@gmail.com > > >>>>>> : > > >>>>>>>>> > > >>>>>>>>> Hi Ivan, > > >>>>>>>>> > > >>>>>>>>> Thank you for your email. My understanding from the jira > > >> issue was > > >>>>> it > > >>>>>>>> will > > >>>>>>>>> be cache level configuration for query default timeout. > > >>>>>>>>> > > >>>>>>>>> I need more info on the usage for this config property and > > >> if it is > > >>>>>>>> shared > > >>>>>>>>> in this jira issue I can make changes or if there is a > > >> separate > > >>>>> jira > > >>>>>>>> issue > > >>>>>>>>> I can assign myself. > > >>>>>>>>> > > >>>>>>>>> > > >>>>>>>>> Regards, > > >>>>>>>>> Saikat > > >>>>>>>>> > > >>>>>>>>> On Mon, Apr 22, 2019 at 5:31 AM Павлухин Иван < > > >> vololo...@gmail.com > > >>>>>> > > >>>>>>>> wrote: > > >>>>>>>>> > > >>>>>>>>>> Hi Saikat, > > >>>>>>>>>> > > >>>>>>>>>> I see that a configuration property is added in PR but I > > >> do not > > >>>>> see > > >>>>>>>>>> how the property is used. Was it done intentionally? > > >>>>>>>>>> > > >>>>>>>>>> Also, we need to decide whether such timeout should be > > >>>>> configured per > > >>>>>>>>>> cache or for all caches in one place. For example, we have > > >>>>> already > > >>>>>>>>>> TransactionConfiguration#setDefaultTxTimeout which is a > > >> global > > >>>>> one. > > >>>>>>>>>> > > >>>>>>>>>> Share you thoughts. > > >>>>>>>>>> > > >>>>>>>>>> вс, 21 апр. 2019 г. в 00:38, Saikat Maitra < > > >>>>> saikat.mai...@gmail.com > > >>>>>>>> : > > >>>>>>>>>>> > > >>>>>>>>>>> Hi, > > >>>>>>>>>>> > > >>>>>>>>>>> I have raised a PR for the below issue. > > >>>>>>>>>>> > > >>>>>>>>>>> IGNITE-7285 Add default query timeout > > >>>>>>>>>>> > > >>>>>>>>>>> PR - https://github.com/apache/ignite/pull/6490 > > >>>>>>>>>>> > > >>>>>>>>>>> Please take a look and share feedback. > > >>>>>>>>>>> > > >>>>>>>>>>> Regards, > > >>>>>>>>>>> Saikat > > >>>>>>>>>> > > >>>>>>>>>> > > >>>>>>>>>> > > >>>>>>>>>> -- > > >>>>>>>>>> Best regards, > > >>>>>>>>>> Ivan Pavlukhin > > >>>>>>>>>> > > >>>>>>>> > > >>>>>>>> > > >>>>>>>> > > >>>>>>>> -- > > >>>>>>>> Best regards, > > >>>>>>>> Ivan Pavlukhin > > >>>>>>>> > > >>>>>>> > > >>>>>>> > > >>>>>>> -- > > >>>>>>> Alexey Kuznetsov > > >>>>>>> > > >>>>> > > >>>>> > > >>>>> > > >>>>> -- > > >>>>> Best regards, > > >>>>> Ivan Pavlukhin > > >>>>> > > >>> > > >>> > > >>> > > >>> -- > > >>> Best regards, > > >>> Ivan Pavlukhin > > >> > > >> > > >> > > >> -- > > >> Best regards, > > >> Ivan Pavlukhin > > >> > > -- Best regards, Ivan Pavlukhin
