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