Re: Potential OOM while iterating over query cursor. Review needed.

[Alexey Goncharuk <al...@gmail.com>]

Folks,

Bumping up the discussion as it is hitting one of the Ignite users.

The change seams reasonable to me, but it is a breaking change and may
affect existing users. Would the community be ok if we change the
QueryCursor#getAll method for scan queries? If not, we should expose the
keepAll() flag to the public API.

пт, 29 июн. 2018 г. в 11:37, Andrey Mashenkov <an...@gmail.com>:

> Hi Igniters,
>
> There is an issue IGNITE-8892 [1] related to OOM during distributed query
> execution.
> This issue is not limited with ScanQuery usage and looks like affected all
> query types.
>
> The use case is quite simple. 1 server and 1 client.
> Client starts scan query with default flags and iterate over cursor.
> If whole query result is not fit to memory - JVM will crashed with OOM,
> but it is not expected as client takes entries one by one and throw out
> them immediately.
> Reproducer is attached to the ticket.
>
> Same query works fine if query starts on server. Seems, we have no
> DistributedQueryFuture in that case and all works fine.
>
> I've found GridCacheDistributedQueryFuture collects all entries and try to
> return the collection via onDone().
> This behaviour turn on with a flag 'keepAll' which is true by default.
> Iterating over cache via cache.iterator() has no OOM issues as we set
> keepAll flag to false.
>
> Why we have keepAll=true by default as seems noone expects future.get()
> will return any data and all queries works through queue in paging mode?
> Will it be ok to get rid of 'allCol' and keepAll flag at all?
>
> I've made a PR and TC looks fine.
> Could someone review it, please?
>
> [1] https://issues.apache.org/jira/browse/IGNITE-8892
>
>
> --
> Best regards,
> Andrey V. Mashenkov
>

Re: Potential OOM while iterating over query cursor. Review needed.

[Yakov Zhdanov <yz...@apache.org>]

Yes! Just deprecate getAll() and change default keepAll for scans to false.

--Yakov

2018-07-18 13:39 GMT+03:00 Alexey Goncharuk <al...@gmail.com>:

> Folks,
>
> There is no need to add getNext() method because the object we are
> discussing is already an iterator. Then, to summarize the solution, we are
> going to deprecate getAll() method and set keepAll flag to false for scan
> query.
>
> Agree?
>
> пн, 16 июл. 2018 г. в 23:40, Dmitriy Setrakyan <ds...@apache.org>:
>
> > On Mon, Jul 16, 2018 at 5:42 PM, Yakov Zhdanov <yz...@apache.org>
> > wrote:
> >
> > > Dmitry, let's have only getNext() same as jdbc. All other shortcuts
> seem
> > to
> > > overload API without adding much value.
> > >
> >
> > Agree. Do you mind creating a ticket?
> >
>

Re: Potential OOM while iterating over query cursor. Review needed.

[Alexey Goncharuk <al...@gmail.com>]

Folks,

There is no need to add getNext() method because the object we are
discussing is already an iterator. Then, to summarize the solution, we are
going to deprecate getAll() method and set keepAll flag to false for scan
query.

Agree?

пн, 16 июл. 2018 г. в 23:40, Dmitriy Setrakyan <ds...@apache.org>:

> On Mon, Jul 16, 2018 at 5:42 PM, Yakov Zhdanov <yz...@apache.org>
> wrote:
>
> > Dmitry, let's have only getNext() same as jdbc. All other shortcuts seem
> to
> > overload API without adding much value.
> >
>
> Agree. Do you mind creating a ticket?
>

Re: Potential OOM while iterating over query cursor. Review needed.

[Dmitriy Setrakyan <ds...@apache.org>]

On Mon, Jul 16, 2018 at 5:42 PM, Yakov Zhdanov <yz...@apache.org> wrote:

> Dmitry, let's have only getNext() same as jdbc. All other shortcuts seem to
> overload API without adding much value.
>

Agree. Do you mind creating a ticket?

Re: Potential OOM while iterating over query cursor. Review needed.

[Yakov Zhdanov <yz...@apache.org>]

Dmitry, let's have only getNext() same as jdbc. All other shortcuts seem to
overload API without adding much value.

--Yakov

2018-07-16 17:33 GMT+03:00 Dmitriy Setrakyan <ds...@apache.org>:

> Well, instead of getFirst(), I would have getNext(). This way we do not
> have to keep the first entry forever, which could present a problem in case
> if entry is too large.
>
> As far as initializing keepAll() to false - completely agree.
>
> D.
>
> On Mon, Jul 16, 2018 at 4:43 PM, Alexey Goncharuk <
> alexey.goncharuk@gmail.com> wrote:
>
> > No objections from my side. Would be nice to receive some feedback from
> > other community members, though, because this is formally a breaking
> > change.
> >
> > пн, 16 июл. 2018 г. в 16:40, Yakov Zhdanov <yz...@apache.org>:
> >
> > > Guys, it seems we need to deprecate getAll() and remove it in 3.0. I
> > think
> > > it is usable only for queries that return 1 row. Every other case needs
> > > iteration. So having getFirst() seems to be better. Thoughts?
> > >
> > > As far as ScanQuery I think we can properly initialize keepAll to false
> > on
> > > scan query instantiation. I am pretty sure none needs getAll() in
> scans.
> > > Alex?
> > >
> > > --
> > > Yakov
> > >
> >
>

Re: Potential OOM while iterating over query cursor. Review needed.

[Dmitriy Setrakyan <ds...@apache.org>]

Well, instead of getFirst(), I would have getNext(). This way we do not
have to keep the first entry forever, which could present a problem in case
if entry is too large.

As far as initializing keepAll() to false - completely agree.

D.

On Mon, Jul 16, 2018 at 4:43 PM, Alexey Goncharuk <
alexey.goncharuk@gmail.com> wrote:

> No objections from my side. Would be nice to receive some feedback from
> other community members, though, because this is formally a breaking
> change.
>
> пн, 16 июл. 2018 г. в 16:40, Yakov Zhdanov <yz...@apache.org>:
>
> > Guys, it seems we need to deprecate getAll() and remove it in 3.0. I
> think
> > it is usable only for queries that return 1 row. Every other case needs
> > iteration. So having getFirst() seems to be better. Thoughts?
> >
> > As far as ScanQuery I think we can properly initialize keepAll to false
> on
> > scan query instantiation. I am pretty sure none needs getAll() in scans.
> > Alex?
> >
> > --
> > Yakov
> >
>

Re: Potential OOM while iterating over query cursor. Review needed.

[Alexey Goncharuk <al...@gmail.com>]

No objections from my side. Would be nice to receive some feedback from
other community members, though, because this is formally a breaking change.

пн, 16 июл. 2018 г. в 16:40, Yakov Zhdanov <yz...@apache.org>:

> Guys, it seems we need to deprecate getAll() and remove it in 3.0. I think
> it is usable only for queries that return 1 row. Every other case needs
> iteration. So having getFirst() seems to be better. Thoughts?
>
> As far as ScanQuery I think we can properly initialize keepAll to false on
> scan query instantiation. I am pretty sure none needs getAll() in scans.
> Alex?
>
> --
> Yakov
>

Re: Potential OOM while iterating over query cursor. Review needed.

[Yakov Zhdanov <yz...@apache.org>]

Guys, it seems we need to deprecate getAll() and remove it in 3.0. I think
it is usable only for queries that return 1 row. Every other case needs
iteration. So having getFirst() seems to be better. Thoughts?

As far as ScanQuery I think we can properly initialize keepAll to false on
scan query instantiation. I am pretty sure none needs getAll() in scans.
Alex?

--
Yakov

Re: Potential OOM while iterating over query cursor. Review needed.

[Alexey Goncharuk <al...@gmail.com>]

Andrey,

Correct me if I am wrong, but my impression was that after the change
cursor#getAll() will return only the last page of the result, not the whole
collection. If public API method semantics is preserved, then no objections
from my side.

ср, 11 июл. 2018 г. в 15:18, Andrey Mashenkov <an...@gmail.com>:

> Alexey,
>
> I saw no issues on TC with this change, and change affect only private API.
> If you think it can break smth, then we can mark keepAll flag as deprecated
> to be deleted in 3.0 and change default value to true.
> I doubt this flag is useful for Ignite internals, and moreover user always
> can wrap query and implement same logic if he will really need such
> behavior.
> So, I vote for removing useless code if it is of course.
>
> ср, 11 июл. 2018 г., 13:26 Alexey Goncharuk <al...@gmail.com>:
>
> > Folks,
> >
> > Bumping up the discussion as it is hitting one of the Ignite users.
> >
> > The change seams reasonable to me, but it is a breaking change and may
> > affect existing users. Would the community be ok if we change the
> > QueryCursor#getAll method for scan queries? If not, we should expose the
> > keepAll() flag to the public API.
> >
> > пт, 29 июн. 2018 г. в 11:37, Andrey Mashenkov <
> andrey.mashenkov@gmail.com
> > >:
> >
> > > Hi Igniters,
> > >
> > > There is an issue IGNITE-8892 [1] related to OOM during distributed
> query
> > > execution.
> > > This issue is not limited with ScanQuery usage and looks like affected
> > all
> > > query types.
> > >
> > > The use case is quite simple. 1 server and 1 client.
> > > Client starts scan query with default flags and iterate over cursor.
> > > If whole query result is not fit to memory - JVM will crashed with OOM,
> > > but it is not expected as client takes entries one by one and throw out
> > > them immediately.
> > > Reproducer is attached to the ticket.
> > >
> > > Same query works fine if query starts on server. Seems, we have no
> > > DistributedQueryFuture in that case and all works fine.
> > >
> > > I've found GridCacheDistributedQueryFuture collects all entries and try
> > to
> > > return the collection via onDone().
> > > This behaviour turn on with a flag 'keepAll' which is true by default.
> > > Iterating over cache via cache.iterator() has no OOM issues as we set
> > > keepAll flag to false.
> > >
> > > Why we have keepAll=true by default as seems noone expects future.get()
> > > will return any data and all queries works through queue in paging
> mode?
> > > Will it be ok to get rid of 'allCol' and keepAll flag at all?
> > >
> > > I've made a PR and TC looks fine.
> > > Could someone review it, please?
> > >
> > > [1] https://issues.apache.org/jira/browse/IGNITE-8892
> > >
> > >
> > > --
> > > Best regards,
> > > Andrey V. Mashenkov
> > >
> >
>

Re: Potential OOM while iterating over query cursor. Review needed.

[Andrey Mashenkov <an...@gmail.com>]

Alexey,

I saw no issues on TC with this change, and change affect only private API.
If you think it can break smth, then we can mark keepAll flag as deprecated
to be deleted in 3.0 and change default value to true.
I doubt this flag is useful for Ignite internals, and moreover user always
can wrap query and implement same logic if he will really need such
behavior.
So, I vote for removing useless code if it is of course.

ср, 11 июл. 2018 г., 13:26 Alexey Goncharuk <al...@gmail.com>:

> Folks,
>
> Bumping up the discussion as it is hitting one of the Ignite users.
>
> The change seams reasonable to me, but it is a breaking change and may
> affect existing users. Would the community be ok if we change the
> QueryCursor#getAll method for scan queries? If not, we should expose the
> keepAll() flag to the public API.
>
> пт, 29 июн. 2018 г. в 11:37, Andrey Mashenkov <andrey.mashenkov@gmail.com
> >:
>
> > Hi Igniters,
> >
> > There is an issue IGNITE-8892 [1] related to OOM during distributed query
> > execution.
> > This issue is not limited with ScanQuery usage and looks like affected
> all
> > query types.
> >
> > The use case is quite simple. 1 server and 1 client.
> > Client starts scan query with default flags and iterate over cursor.
> > If whole query result is not fit to memory - JVM will crashed with OOM,
> > but it is not expected as client takes entries one by one and throw out
> > them immediately.
> > Reproducer is attached to the ticket.
> >
> > Same query works fine if query starts on server. Seems, we have no
> > DistributedQueryFuture in that case and all works fine.
> >
> > I've found GridCacheDistributedQueryFuture collects all entries and try
> to
> > return the collection via onDone().
> > This behaviour turn on with a flag 'keepAll' which is true by default.
> > Iterating over cache via cache.iterator() has no OOM issues as we set
> > keepAll flag to false.
> >
> > Why we have keepAll=true by default as seems noone expects future.get()
> > will return any data and all queries works through queue in paging mode?
> > Will it be ok to get rid of 'allCol' and keepAll flag at all?
> >
> > I've made a PR and TC looks fine.
> > Could someone review it, please?
> >
> > [1] https://issues.apache.org/jira/browse/IGNITE-8892
> >
> >
> > --
> > Best regards,
> > Andrey V. Mashenkov
> >
>