ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Ivan Pavlukhina <vololo...@gmail.com>
Subject Re: IGNITE-7285 Add default query timeout
Date Tue, 07 May 2019 05:30:07 GMT
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.maitra@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 Павлухин Иван <vololo100@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, Павлухин Иван <vololo100@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.maitra@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 Павлухин Иван <vololo100@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.maitra@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 <
>> akuznetsov@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 Павлухин Иван <
>> vololo100@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.maitra@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 Павлухин Иван
<
>> vololo100@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.maitra@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
>> 

Mime
View raw message