Correct, this also needs to be fixed. On Mon, Aug 27, 2018 at 5:17 PM Evgenii Zhuravlev <e.zhuravlev...@gmail.com> wrote:
> Vladimir, > > Then it looks like we send wrong EVT_CACHE_QUERY_EXECUTED events too, since > we send it for the same cache, as in your example. > > Evgenii > > > > пн, 27 авг. 2018 г. в 13:20, Vladimir Ozerov <voze...@gridgain.com>: > > > Folks, > > > > Can we leave query details metrics API unchanged, and simply fix how we > > record them? Historically it worked as follows: > > 1) IgniteCache.query() is invoked > > 2) Query is executed, possibly on other caches! > > 3) Metric is incremented for cache from p.1 > > > > I.e. our query detail metrics were broken since their first days. I > propose > > to fix them as follows: > > 1) Any SQL query() method is invoked (see GridQueryProcessor) > > 2) We collect all cache IDs during parsing (this already happens) > > 3) Record event for all cache IDs through > > GridCacheQueryManager#collectMetrics > > > > This appears to be as the simplest and consistent solution to the > problem. > > > > On Tue, Aug 21, 2018 at 1:09 PM Evgenii Zhuravlev < > > e.zhuravlev...@gmail.com> > > wrote: > > > > > Hi Alex, > > > > > > I agree that we can't move all metrics to ignite.metrics() and SPI > > metrics > > > is a good example here. I propose to move at least dataRegionMetrics, > > > dataStorageMetrics to it, since the main API class is not a good place > > for > > > such things. If we will decide to choose option 1 from my first > message, > > > then, I will also add QueryDetailMetrics for cacheless queries to this > > new > > > class. > > > > > > пн, 20 авг. 2018 г. в 23:28, Alex Plehanov <plehanov.a...@gmail.com>: > > > > > > > Hi Evgeny, > > > > > > > > Do you propose to move into IgniteMetrics absolutely all Ignite > metrics > > > or > > > > just dataRegionMetrics, dataStorageMetrics and queryDetailMetrics? I > > > think > > > > you can't move all metrics into one place. Pluggable components and > > > > different SPI implementations may have their own metric sets, and > > perhaps > > > > it's not such a good idea to try to fit them in one common fixed > > > interface. > > > > > > > > 2018-08-20 18:14 GMT+03:00 Evgenii Zhuravlev < > e.zhuravlev...@gmail.com > > >: > > > > > > > > > As for now, metrics are accumulated for cache only when the query > is > > > run > > > > > directly over this cache, for example, using ignite.cache("Some > > > > > cache").sqlFieldsQuery("select ... from .."). When a query is > started > > > > using > > > > > other APIs, it doesn't detect cache, to which this table belongs > and > > > > > doesn't save any metrics. > > > > > > > > > > > > > > > > > > > > 2018-08-17 16:44 GMT+03:00 Vladimir Ozerov <voze...@gridgain.com>: > > > > > > > > > > > Query is not executed on specific cache. It is executed on many > > > caches. > > > > > > > > > > > > On Fri, Aug 17, 2018 at 6:10 AM Dmitriy Setrakyan < > > > > dsetrak...@apache.org > > > > > > > > > > > > wrote: > > > > > > > > > > > > > But internally the SQL query still runs on some cache, no? What > > > > happens > > > > > > to > > > > > > > the metrics accumulated on that cache? > > > > > > > > > > > > > > D. > > > > > > > > > > > > > > On Thu, Aug 16, 2018, 18:51 Alexey Kuznetsov < > > > akuznet...@apache.org> > > > > > > > wrote: > > > > > > > > > > > > > > > Dima, > > > > > > > > > > > > > > > > "cache-less" means that SQL executed directly on SQL engine. > > > > > > > > > > > > > > > > In previous version of Ignite we execute queries via cache: > > > > > > > > > > > > > > > > ignite.cache("Some cache").sqlFieldsQuery("select ... from > ..") > > > > > > > > > > > > > > > > In current Ignite we can execute query directly without using > > > cache > > > > > as > > > > > > > > "gateway". > > > > > > > > > > > > > > > > And if we execute query directly, metrics not update. > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > On Fri, Aug 17, 2018 at 4:21 AM Dmitriy Setrakyan < > > > > > > dsetrak...@apache.org > > > > > > > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > Evgeny, what is a "cache-less" SQL query? > > > > > > > > > > > > > > > > > > D. > > > > > > > > > > > > > > > > > > On Thu, Aug 16, 2018 at 6:36 AM, Evgenii Zhuravlev < > > > > > > > > > e.zhuravlev...@gmail.com > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > Hi Igniters, > > > > > > > > > > > > > > > > > > > > I've started to work on adding QueryDetailMetrics for > > > > cache-less > > > > > > SQL > > > > > > > > > > queries(issue > > > > https://issues.apache.org/jira/browse/IGNITE-6677) > > > > > > and > > > > > > > > > found > > > > > > > > > > that it's required to change API. I don't think that > adding > > > > > methods > > > > > > > > like > > > > > > > > > > queryDetailMetrics, resetQueryDetailMetrics, as in > > > IgniteCache > > > > to > > > > > > > > Ignite > > > > > > > > > > class is a good idea. So, I see 2 possible solutions > here: > > > > > > > > > > > > > > > > > > > > 1. Create IgniteMetrics(ignite.metrics()) and move > metrics > > > from > > > > > > > > > > Ignite(like dataRegionMetrics and dataStorageMetrics) and > > > add a > > > > > new > > > > > > > > > > metric "queryDetailMetrics" to it. Of course, old methods > > > will > > > > be > > > > > > > > > > deprecated. > > > > > > > > > > > > > > > > > > > > 2. Finally create Ignite.sql() API, which was already > > > discussed > > > > > > here: > > > > > > > > > > http://apache-ignite-developers.2346864.n4.nabble. > > > > > > > > > > > > com/Rethink-native-SQL-API-in-Apache-Ignite-2-0-td14335.html > > > > > > > > > > and place "queryDetailMetrics" metric there. Here is the > > > ticket > > > > > for > > > > > > > > this > > > > > > > > > > change: > https://issues.apache.org/jira/browse/IGNITE-4701 > > > > > > > > > > > > > > > > > > > > Personally, I think that the second solution looks better > > in > > > > this > > > > > > > case, > > > > > > > > > > however, moving dataRegionMetrics and dataStorageMetrics > to > > > > > > > > > > ignite.matrics() is still a good idea - IMO, Ignite class > > is > > > > not > > > > > > the > > > > > > > > > right > > > > > > > > > > place for them - we shouldn't change our main API class > so > > > > often. > > > > > > > > > > > > > > > > > > > > What do you think? > > > > > > > > > > > > > > > > > > > > Thank you, > > > > > > > > > > Evgenii > > > > > > > > > > > > > > > > > > > > > > > > > > > > -- > > > > > > > > > Alexey Kuznetsov > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >