[
https://issues.apache.org/jira/browse/IGNITE-10754?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16738110#comment-16738110
]
Vladimir Ozerov commented on IGNITE-10754:
------------------------------------------
[~jooger], my comments:
# We should not deprecate existing functional, as there is nothing wrong with -
it just works on a different level. Moreover, it is concerned with non-SQL
queries (e.g. Scan), while this ticket is only about SQL.
# No need for interfaces and adapters for {{QueryHistoryMetrics}}. Single
concrete class would e enough
# {{QueryHistoryMetrics}} should not be located in public package, as it will
only be accessible from SQL and web console.
# {{QueryHistoryManager}} - it is overkill to have separate manager to track
history. Let's merge it with running queries manager.
# No need to expose {{QueryHistoryManager}} from {{GridQueryProcessor}}
# {{QueryHistoryManager}} - {{clear}} operation cannot be implemented this way,
as it is not thread-safe and may lead to inconsistent state. Instead, both data
structures should be encapsulated in some volatile composite object. And
{{clear}} should simply create new such object. This way we will avoid races
when data structure will contain queries created before {{clear}}
# {{QueryHistoryManager.collectMetrics}} - {{metricsFull}} should be called
only when it is need - immediately before shrink
# I am not quite understand the line {{if (qryMetrics.get(entry.key()) !=
entry)}} - what is the purpose? From what I see, {{shrink}} method first
removes an entry from queue, and then from the map. So this check looks
redundant to me. Otherwise, if we really need it, why this check is not
performed in {{else if (removeLink(node))}} branch?
# {{org.apache.ignite.internal.processors.query.QueryHistoryManager#explain}} -
what is wrong with EXPLAIN queries that we decided to exclude them?
# {{QueryHistoryManager.queryHistoryMetrics}} - implementation looks
inefficient: why do we create intermediate array only to covert it to the list
later on? Moreover, why do we covert queue to list before that (which creates
another intermediate ArrayList internally)? Last, queue -> list coversion is
not safe in terms of duplicates - you may get the same entry twice in case of
concurrent query execution. Why can't we just iterate over the map without any
intermediate collections?
# {{RunningQueryManager.unregister}} - we should log only SQL queries, but this
doesn't seem to be the case
# {{RunningQueryManager.unregister}} - no need to add queries to history in
case of stop
> Query history statistics API
> ----------------------------
>
> Key: IGNITE-10754
> URL: https://issues.apache.org/jira/browse/IGNITE-10754
> Project: Ignite
> Issue Type: Task
> Components: sql
> Reporter: Yury Gerzhedovich
> Assignee: Yury Gerzhedovich
> Priority: Major
> Labels: iep-29, monitoring
>
> As of now we have query statistics
> (*_org.apache.ignite.IgniteCache#queryMetrics_*) , but have few issues.
> 1) Duration execution it just time between start execution and return cursor
> to client and doesn't include all life time of query.
> 2) It doesn't know about multistatement queries. Such queries participate in
> statistics as single query without splitting.
> 3) API to access the statistics expose as depend on cache, however query
> don't have such dependency.
>
> Need to create parallel similar realization as we already have.
> Use new infrastructure of tracking running queries developed under
> IGNITE-10621 and update statistics on unregister phase.
> Expose API on upper level then it placed now. Right place will be written
> later.
>
> Old API will be marked as deprecated.
>
--
This message was sent by Atlassian JIRA
(v7.6.3#76005)
