ignite-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Dmitriy Setrakyan <dsetrak...@apache.org>
Subject Fwd: Queries API considerations.
Date Fri, 20 Mar 2015 23:49:45 GMT
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