ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Dmitriy Setrakyan <dsetrak...@apache.org>
Subject Re: Queries API considerations.
Date Sat, 21 Mar 2015 09:53:43 GMT
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 <vozerov@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 <dsetrakyan@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 <vozerov@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?
> > >
> >
>

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message