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

2018-07-18 Thread Yakov Zhdanov
Yes! Just deprecate getAll() and change default keepAll for scans to false. --Yakov 2018-07-18 13:39 GMT+03:00 Alexey Goncharuk : > 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 t

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

2018-07-18 Thread Alexey Goncharuk
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 : > On Mon,

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

2018-07-16 Thread Dmitriy Setrakyan
On Mon, Jul 16, 2018 at 5:42 PM, Yakov Zhdanov 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.

2018-07-16 Thread Yakov Zhdanov
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 : > Well, instead of getFirst(), I would have getNext(). This way we do not > have to keep the first entry forever, which could

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

2018-07-16 Thread Dmitriy Setrakyan
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.g

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

2018-07-16 Thread Alexey Goncharuk
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 : > Guys, it seems we need to deprecate getAll() and remove it in 3.0. I think > it is usable only for qu

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

2018-07-16 Thread Yakov Zhdanov
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 ins

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

2018-07-11 Thread Alexey Goncharuk
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 : > Al

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

2018-07-11 Thread Andrey Mashenkov
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

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

2018-07-11 Thread Alexey Goncharuk
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() fl