ignite-dev mailing list archives

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



-- 
Best regards,
Ivan Pavlukhin

Mime
View raw message