You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ignite.apache.org by Vladimir Ozerov <vo...@gridgain.com> on 2015/03/20 22:38:51 UTC
Queries API considerations.
Guys,
The more I look into our queries API, the more questions appears.
1) User can either iterate over result set, or get all returned values. In
the latter case query is automatically closed after "getAll()" finished.
But this is not the case for iterator. At least this is not mentioned is
JavaDocs. For this reason instead of doing:
for (CacheEntry entry : cache.query(...)) {
}
user will have to do the following:
try (Query qry = cache.query(...)) {
for (CacheEntry entry : qry) {
}
}
Looks like we have to close query whenever end of results stream is
reached, irrespective of whether this was getAll(), or iterator end.
Thoughts?
2) Initial query inside contnuous query appears to be fundamentally wrong
concept. Continuous query is about events. Regular query is about iteration
over existing enries. Why do we mix them? They have nothing in common.
If user want to run "initial query" ... just let him do that by hand after
continuous query is started.
Furthermore, IgniteCache.query() accepts SQL and TEXT queries.
ContinuousQuery.initialQuery accepts SQL, TEXT and FIELDS queries, WTF? How
are we going to explain user this semantics?
Furthermore, from running regular queries user knows that when
QueryCursor.getAll() is called, query is closed. But will continuous query
be closed when the user call getAll() on initial query result? No, it
won't. Another inconsistency.
Furthermore, when a QueryCursor returned from IgniteCache.continuousQuery()
method is closed, it closes both continuous and initial queries. But what
if I want to release initial query resources, but leave continuous query
running? It is impossible at the moment - either keep open both, or close
both.
I propose to remove initial query together with a cursor returned from
IgniteCache.continuousQuery(). It has no value. Instead, continuousQuery()
method should return a kind of "handle" which can be closed (e.g. extends
AutoCloseable), nothing more.
3) ContinuousQuery extends Query. Query have only one property - page size.
Continuous query does not use this. How come we expose not used property to
the user? ContinuousQuery should not extend Query as it has nothing common
with it except of a word "query" in name.
Also, as we distinguish entries queries (SQL, TEXT) and fields queries,
probably it does not make sense to make them share the same Query class.
What if we define the following hierarchy:
- Query, SqlQuery extends Query, TextQuery extends Query;
- SqlFieldsQuery
- ContinuousQuery
It will map perfectly to existing methods:
- query(Query)
- fieldsQuery(SqlFIeldsQuery)
- continuousQuery(ContinuousQuery).
4) Finally, instead of having paired methods "queryXXX" and
"localQueryXXX", we can add "local" property to query classes and remove
"localQueryXXX" methods from API.
Thoughts?
Re: Fwd: Queries API considerations.
Posted by Dmitriy Setrakyan <ds...@apache.org>.
Yes, agreed.
On Sat, Mar 21, 2015 at 2:13 AM, Branko Čibej <br...@apache.org> wrote:
> On 21.03.2015 00:49, Dmitriy Setrakyan wrote:
> > This is kind-of last minute,
>
> Point of order: The community has made no decision on release schedule,
> so there can be nothing 'last-minute' to the proposal. Even if there
> were a schedule, it would be a matter of internal consensus, not
> external constraints, and thus easily changed to accommodate an
> important change. But I've seen no consensus on schedule emerge from
> discussions on this list.
>
> -- Brane
>
>
Re: Fwd: Queries API considerations.
Posted by Branko Čibej <br...@apache.org>.
On 21.03.2015 00:49, Dmitriy Setrakyan wrote:
> This is kind-of last minute,
Point of order: The community has made no decision on release schedule,
so there can be nothing 'last-minute' to the proposal. Even if there
were a schedule, it would be a matter of internal consensus, not
external constraints, and thus easily changed to accommodate an
important change. But I've seen no consensus on schedule emerge from
discussions on this list.
-- Brane
Re: Queries API considerations.
Posted by Dmitriy Setrakyan <ds...@apache.org>.
Vladimir,
Please take a look at the changes in ignite-45. In this branch we have only
1 query method on IgniteCache:
public <T> QueryCursor<T> query(Query<T>)
Note how the API no longer has sqlFieldsQuery(...) method or
localQuery(...) methods (mainly due to your suggestions).
Also note how the Query interface itself declares its return type via
generics.I think it is the right approach and should fit all the queries in
Ignite, including the ContinuousQuery. I think you would agree that this
API is a lot cleaner than before.
Now that we don't have any special APIs for any query types, I would
strongly prefer not to make a special case for ContinuousQuery either. I do
see your point about QueryCursor.close() and getAll() methods for the
ContinuousQuery not being straight forward, but even with that, I still
prefer the API with one "query(...)" method for all types of queries,
without special cases.
D.
On Fri, Mar 20, 2015 at 11:45 PM, Vladimir Ozerov <vo...@gridgain.com>
wrote:
> Dima,
>
> Let me clarify my points about continuous query. Actually, there is only
> point of confusion here - initial query. If we remove it, we will not have
> a cursor, and will not have a problem with several "close" methods.
>
> The we look closely to how initial query is implemented, then we see that
> semantically current implementation is:
> - Start listening events.
> - Execute initial query through IgniteCache.query() method
> - Wrap itniial query cursor into another class (which breakes getAll()
> semantics)
> - Return.
>
> So, if user want to execute "initial query", he can:
> 1) Use "initialQuery":
> ContinuousQuery cont = new ContinuousQuery();
> cont.setInintialQuery(initialQuery);
> QueryCursor = cache.continuousQuery(cont);
>
> 2) Or call cache API twice:
> ContinuousQuery cont = new ContinuousQuery();
> cache.continuousQuery(cont);
> QueryCursor = cache.query(initialQuery);
>
> These two code chunks will yield in absolutely the same results from user
> perspective. But in the latter case API is more clean and consistent from
> user point of view: "here I start listening for events with
> \continuousQuery\ call, and here I iterate over existing entries with
> \query\ call".
>
> This is why I'am pushing idea of removing initialQuery:
> 1) Breakes QueryCursor.getAll() semantics;
> 2) Breakes QueryCursor.close() semantics;
> 3) Mixes "cache entry" and "cache entry event" concepts. User is not able
> to "feed" initial query results to continuous query listener, because
> initial query results are either "CacheEntry" or "List", and continuous
> query listener accepts "CacheEntryEvent". So separate handlers for events
> and initial results are required from user anyway.
> 4) And the most important: with all these drawbacks in place, it adds
> virtually no value to API because "initial query" semantics is already
> achieveable with two consequent calls: continuousQuery() for events +
> query() for iteration over existing entries.
>
>
> On Sat, Mar 21, 2015 at 2:49 AM, Dmitriy Setrakyan <ds...@apache.org>
> wrote:
>
> > Vladimir,
> >
> > This is kind-of last minute, but you have brought up some good points. I
> am
> > not sure if I agree with everything though. Here are my comments:
> >
> > 1. We have 2 query methods, one for Query and another for SqlFieldsQuery,
> > and every time we add a new type of query in the current design we have
> to
> > add a new method. Let's us try to have just one query methods with return
> > type specified as generic, like so:
> >
> > <T> QueryCursor<T> query(Query<T>)
> >
> > If we do this, then all query methods have to conform to the same API.
> >
> > 2. I really like your idea about removing localQuery methods. Let's try
> > making that change before the release.
> >
> > 3. I do believe that having initial-query and returning QueryCursor from
> > continuous query is correct design. In the absence of it, user would have
> > to know intricate internal implementation details in order to register
> > listener correctly to prevent missing events.
> >
> > 4. As far as method QueryCursor.close() closing the whole continuous
> query,
> > I also think it is the correct approach, although, I do agree that it may
> > be confusing. In the absence of this behavior, we would have to have 2
> > close() methods, which would be, arguably, equally confusing.
> >
> > I filed a ticket to address these changes here:
> > https://issues.apache.org/jira/browse/IGNITE-543
> >
> > D.
> >
> > On Fri, Mar 20, 2015 at 2:38 PM, Vladimir Ozerov <vo...@gridgain.com>
> > wrote:
> >
> > > Guys,
> > >
> > > The more I look into our queries API, the more questions appears.
> > >
> > > 1) User can either iterate over result set, or get all returned values.
> > In
> > > the latter case query is automatically closed after "getAll()"
> finished.
> > > But this is not the case for iterator. At least this is not mentioned
> is
> > > JavaDocs. For this reason instead of doing:
> > >
> > > for (CacheEntry entry : cache.query(...)) {
> > > }
> > >
> > > user will have to do the following:
> > >
> > > try (Query qry = cache.query(...)) {
> > > for (CacheEntry entry : qry) {
> > > }
> > > }
> > >
> > > Looks like we have to close query whenever end of results stream is
> > > reached, irrespective of whether this was getAll(), or iterator end.
> > > Thoughts?
> > >
> > > 2) Initial query inside contnuous query appears to be fundamentally
> wrong
> > > concept. Continuous query is about events. Regular query is about
> > iteration
> > > over existing enries. Why do we mix them? They have nothing in common.
> > > If user want to run "initial query" ... just let him do that by hand
> > after
> > > continuous query is started.
> > >
> > > Furthermore, IgniteCache.query() accepts SQL and TEXT queries.
> > > ContinuousQuery.initialQuery accepts SQL, TEXT and FIELDS queries, WTF?
> > How
> > > are we going to explain user this semantics?
> > >
> > > Furthermore, from running regular queries user knows that when
> > > QueryCursor.getAll() is called, query is closed. But will continuous
> > query
> > > be closed when the user call getAll() on initial query result? No, it
> > > won't. Another inconsistency.
> > >
> > > Furthermore, when a QueryCursor returned from
> > IgniteCache.continuousQuery()
> > > method is closed, it closes both continuous and initial queries. But
> what
> > > if I want to release initial query resources, but leave continuous
> query
> > > running? It is impossible at the moment - either keep open both, or
> close
> > > both.
> > >
> > > I propose to remove initial query together with a cursor returned from
> > > IgniteCache.continuousQuery(). It has no value. Instead,
> > continuousQuery()
> > > method should return a kind of "handle" which can be closed (e.g.
> extends
> > > AutoCloseable), nothing more.
> > >
> > > 3) ContinuousQuery extends Query. Query have only one property - page
> > size.
> > > Continuous query does not use this. How come we expose not used
> property
> > to
> > > the user? ContinuousQuery should not extend Query as it has nothing
> > common
> > > with it except of a word "query" in name.
> > >
> > > Also, as we distinguish entries queries (SQL, TEXT) and fields queries,
> > > probably it does not make sense to make them share the same Query
> class.
> > >
> > > What if we define the following hierarchy:
> > > - Query, SqlQuery extends Query, TextQuery extends Query;
> > > - SqlFieldsQuery
> > > - ContinuousQuery
> > >
> > > It will map perfectly to existing methods:
> > > - query(Query)
> > > - fieldsQuery(SqlFIeldsQuery)
> > > - continuousQuery(ContinuousQuery).
> > >
> > > 4) Finally, instead of having paired methods "queryXXX" and
> > > "localQueryXXX", we can add "local" property to query classes and
> remove
> > > "localQueryXXX" methods from API.
> > >
> > > Thoughts?
> > >
> >
>
Re: Queries API considerations.
Posted by Vladimir Ozerov <vo...@gridgain.com>.
Dima,
Let me clarify my points about continuous query. Actually, there is only
point of confusion here - initial query. If we remove it, we will not have
a cursor, and will not have a problem with several "close" methods.
The we look closely to how initial query is implemented, then we see that
semantically current implementation is:
- Start listening events.
- Execute initial query through IgniteCache.query() method
- Wrap itniial query cursor into another class (which breakes getAll()
semantics)
- Return.
So, if user want to execute "initial query", he can:
1) Use "initialQuery":
ContinuousQuery cont = new ContinuousQuery();
cont.setInintialQuery(initialQuery);
QueryCursor = cache.continuousQuery(cont);
2) Or call cache API twice:
ContinuousQuery cont = new ContinuousQuery();
cache.continuousQuery(cont);
QueryCursor = cache.query(initialQuery);
These two code chunks will yield in absolutely the same results from user
perspective. But in the latter case API is more clean and consistent from
user point of view: "here I start listening for events with
\continuousQuery\ call, and here I iterate over existing entries with
\query\ call".
This is why I'am pushing idea of removing initialQuery:
1) Breakes QueryCursor.getAll() semantics;
2) Breakes QueryCursor.close() semantics;
3) Mixes "cache entry" and "cache entry event" concepts. User is not able
to "feed" initial query results to continuous query listener, because
initial query results are either "CacheEntry" or "List", and continuous
query listener accepts "CacheEntryEvent". So separate handlers for events
and initial results are required from user anyway.
4) And the most important: with all these drawbacks in place, it adds
virtually no value to API because "initial query" semantics is already
achieveable with two consequent calls: continuousQuery() for events +
query() for iteration over existing entries.
On Sat, Mar 21, 2015 at 2:49 AM, Dmitriy Setrakyan <ds...@apache.org>
wrote:
> Vladimir,
>
> This is kind-of last minute, but you have brought up some good points. I am
> not sure if I agree with everything though. Here are my comments:
>
> 1. We have 2 query methods, one for Query and another for SqlFieldsQuery,
> and every time we add a new type of query in the current design we have to
> add a new method. Let's us try to have just one query methods with return
> type specified as generic, like so:
>
> <T> QueryCursor<T> query(Query<T>)
>
> If we do this, then all query methods have to conform to the same API.
>
> 2. I really like your idea about removing localQuery methods. Let's try
> making that change before the release.
>
> 3. I do believe that having initial-query and returning QueryCursor from
> continuous query is correct design. In the absence of it, user would have
> to know intricate internal implementation details in order to register
> listener correctly to prevent missing events.
>
> 4. As far as method QueryCursor.close() closing the whole continuous query,
> I also think it is the correct approach, although, I do agree that it may
> be confusing. In the absence of this behavior, we would have to have 2
> close() methods, which would be, arguably, equally confusing.
>
> I filed a ticket to address these changes here:
> https://issues.apache.org/jira/browse/IGNITE-543
>
> D.
>
> On Fri, Mar 20, 2015 at 2:38 PM, Vladimir Ozerov <vo...@gridgain.com>
> wrote:
>
> > Guys,
> >
> > The more I look into our queries API, the more questions appears.
> >
> > 1) User can either iterate over result set, or get all returned values.
> In
> > the latter case query is automatically closed after "getAll()" finished.
> > But this is not the case for iterator. At least this is not mentioned is
> > JavaDocs. For this reason instead of doing:
> >
> > for (CacheEntry entry : cache.query(...)) {
> > }
> >
> > user will have to do the following:
> >
> > try (Query qry = cache.query(...)) {
> > for (CacheEntry entry : qry) {
> > }
> > }
> >
> > Looks like we have to close query whenever end of results stream is
> > reached, irrespective of whether this was getAll(), or iterator end.
> > Thoughts?
> >
> > 2) Initial query inside contnuous query appears to be fundamentally wrong
> > concept. Continuous query is about events. Regular query is about
> iteration
> > over existing enries. Why do we mix them? They have nothing in common.
> > If user want to run "initial query" ... just let him do that by hand
> after
> > continuous query is started.
> >
> > Furthermore, IgniteCache.query() accepts SQL and TEXT queries.
> > ContinuousQuery.initialQuery accepts SQL, TEXT and FIELDS queries, WTF?
> How
> > are we going to explain user this semantics?
> >
> > Furthermore, from running regular queries user knows that when
> > QueryCursor.getAll() is called, query is closed. But will continuous
> query
> > be closed when the user call getAll() on initial query result? No, it
> > won't. Another inconsistency.
> >
> > Furthermore, when a QueryCursor returned from
> IgniteCache.continuousQuery()
> > method is closed, it closes both continuous and initial queries. But what
> > if I want to release initial query resources, but leave continuous query
> > running? It is impossible at the moment - either keep open both, or close
> > both.
> >
> > I propose to remove initial query together with a cursor returned from
> > IgniteCache.continuousQuery(). It has no value. Instead,
> continuousQuery()
> > method should return a kind of "handle" which can be closed (e.g. extends
> > AutoCloseable), nothing more.
> >
> > 3) ContinuousQuery extends Query. Query have only one property - page
> size.
> > Continuous query does not use this. How come we expose not used property
> to
> > the user? ContinuousQuery should not extend Query as it has nothing
> common
> > with it except of a word "query" in name.
> >
> > Also, as we distinguish entries queries (SQL, TEXT) and fields queries,
> > probably it does not make sense to make them share the same Query class.
> >
> > What if we define the following hierarchy:
> > - Query, SqlQuery extends Query, TextQuery extends Query;
> > - SqlFieldsQuery
> > - ContinuousQuery
> >
> > It will map perfectly to existing methods:
> > - query(Query)
> > - fieldsQuery(SqlFIeldsQuery)
> > - continuousQuery(ContinuousQuery).
> >
> > 4) Finally, instead of having paired methods "queryXXX" and
> > "localQueryXXX", we can add "local" property to query classes and remove
> > "localQueryXXX" methods from API.
> >
> > Thoughts?
> >
>
Fwd: Queries API considerations.
Posted by Dmitriy Setrakyan <ds...@apache.org>.
Vladimir,
This is kind-of last minute, but you have brought up some good points. I am
not sure if I agree with everything though. Here are my comments:
1. We have 2 query methods, one for Query and another for SqlFieldsQuery,
and every time we add a new type of query in the current design we have to
add a new method. Let's us try to have just one query methods with return
type specified as generic, like so:
<T> QueryCursor<T> query(Query<T>)
If we do this, then all query methods have to conform to the same API.
2. I really like your idea about removing localQuery methods. Let's try
making that change before the release.
3. I do believe that having initial-query and returning QueryCursor from
continuous query is correct design. In the absence of it, user would have
to know intricate internal implementation details in order to register
listener correctly to prevent missing events.
4. As far as method QueryCursor.close() closing the whole continuous query,
I also think it is the correct approach, although, I do agree that it may
be confusing. In the absence of this behavior, we would have to have 2
close() methods, which would be, arguably, equally confusing.
I filed a ticket to address these changes here:
https://issues.apache.org/jira/browse/IGNITE-543
D.
On Fri, Mar 20, 2015 at 2:38 PM, Vladimir Ozerov <vo...@gridgain.com>
wrote:
> Guys,
>
> The more I look into our queries API, the more questions appears.
>
> 1) User can either iterate over result set, or get all returned values. In
> the latter case query is automatically closed after "getAll()" finished.
> But this is not the case for iterator. At least this is not mentioned is
> JavaDocs. For this reason instead of doing:
>
> for (CacheEntry entry : cache.query(...)) {
> }
>
> user will have to do the following:
>
> try (Query qry = cache.query(...)) {
> for (CacheEntry entry : qry) {
> }
> }
>
> Looks like we have to close query whenever end of results stream is
> reached, irrespective of whether this was getAll(), or iterator end.
> Thoughts?
>
> 2) Initial query inside contnuous query appears to be fundamentally wrong
> concept. Continuous query is about events. Regular query is about iteration
> over existing enries. Why do we mix them? They have nothing in common.
> If user want to run "initial query" ... just let him do that by hand after
> continuous query is started.
>
> Furthermore, IgniteCache.query() accepts SQL and TEXT queries.
> ContinuousQuery.initialQuery accepts SQL, TEXT and FIELDS queries, WTF? How
> are we going to explain user this semantics?
>
> Furthermore, from running regular queries user knows that when
> QueryCursor.getAll() is called, query is closed. But will continuous query
> be closed when the user call getAll() on initial query result? No, it
> won't. Another inconsistency.
>
> Furthermore, when a QueryCursor returned from IgniteCache.continuousQuery()
> method is closed, it closes both continuous and initial queries. But what
> if I want to release initial query resources, but leave continuous query
> running? It is impossible at the moment - either keep open both, or close
> both.
>
> I propose to remove initial query together with a cursor returned from
> IgniteCache.continuousQuery(). It has no value. Instead, continuousQuery()
> method should return a kind of "handle" which can be closed (e.g. extends
> AutoCloseable), nothing more.
>
> 3) ContinuousQuery extends Query. Query have only one property - page size.
> Continuous query does not use this. How come we expose not used property to
> the user? ContinuousQuery should not extend Query as it has nothing common
> with it except of a word "query" in name.
>
> Also, as we distinguish entries queries (SQL, TEXT) and fields queries,
> probably it does not make sense to make them share the same Query class.
>
> What if we define the following hierarchy:
> - Query, SqlQuery extends Query, TextQuery extends Query;
> - SqlFieldsQuery
> - ContinuousQuery
>
> It will map perfectly to existing methods:
> - query(Query)
> - fieldsQuery(SqlFIeldsQuery)
> - continuousQuery(ContinuousQuery).
>
> 4) Finally, instead of having paired methods "queryXXX" and
> "localQueryXXX", we can add "local" property to query classes and remove
> "localQueryXXX" methods from API.
>
> Thoughts?
>