IGNITE-29011 SQL: Add result set size metrics for queries - #13517
IGNITE-29011 SQL: Add result set size metrics for queries#13517alex-plekhanov wants to merge 5 commits into
Conversation
zstan
left a comment
zstan
left a comment
There was a problem hiding this comment.
I fill some Nit comments plz check them
| assertEquals(0, values[i]); | ||
|
|
||
| // Verify max value on the initiating node. | ||
| assertEquals(500L, resultSetSizeMax(initNode).value()); |
There was a problem hiding this comment.
probably it`s better to use constant here just for test clarification ?
and constant here:
for (int limit : new int[] {0, 1, 5, 50, 500 <-- change for constant ? })
There was a problem hiding this comment.
Constant for range or constant for each value? I think both is redundant.
|
|
||
| /** */ | ||
| IgniteH2Indexing h2() { | ||
| return h2; |
There was a problem hiding this comment.
why do we need 'whole' IgniteH2Indexing access here ? I think better give only necessary i.e. : return h2.runningQueryManager();
There was a problem hiding this comment.
But it looks not very consistent, runing query manager is not relied to reduce executor, but reduce executor is a part of indexing, so it's more correct to bind these two components.
| "on this node. This metric number included in the general 'failed' metric."); | ||
|
|
||
| resultSetSizeHistogram = userMetrics.histogram("resultSetSizeHistogram", | ||
| new long[] {0, 1, 10, 100, 1_000, 10_000, 100_000, 1_000_000}, |
There was a problem hiding this comment.
can we also give a Billion here ? In distributed huge cluster it`s normal i think
There was a problem hiding this comment.
Fetching more than million entries for sql query it's not normal even if cluster contains billions (I think it's even not ok to fetch more than 1000 entries).
All fetches with more than million entries go to last bucket, so they will not be missed.
Buckets are tunable, if someone think that it's ok to fetch million entries but not ok to fetch billion entries, it can be configured.
| assertEquals(500L, resultSetSizeMax(initNode).value()); | ||
|
|
||
| // Verify all other server nodes have zero metrics. | ||
| for (int i = 1; i < nodeCount(); i++) { |
There was a problem hiding this comment.
= 1 rely on GridAbstractTest#startGrids i`d prefer to filter by node name.
There was a problem hiding this comment.
Not sure I get you right, but fixed something
| /** | ||
| * Tests for result set size histogram and max result set size metrics. | ||
| */ | ||
| public class ResultSetSizeMetricsTest extends AbstractMultiEngineIntegrationTest { |
There was a problem hiding this comment.
I see SqlDiagnosticIntegrationTest probably it about diagnostic too ? I don`t know here ...
There was a problem hiding this comment.
SqlDiagnosticIntegrationTest - is about Calcite only engine. The new test uses both engines.
2 participants
Thank you for submitting the pull request to the Apache Ignite.
In order to streamline the review of the contribution
we ask you to ensure the following steps have been taken:
The Contribution Checklist
The description explains WHAT and WHY was made instead of HOW.
The following pattern must be used:
IGNITE-XXXX Change summarywhereXXXX- number of JIRA issue.(see the Maintainers list)
the
green visaattached to the JIRA ticket (see tabPR Checkat TC.Bot - Instance 1 or TC.Bot - Instance 2)Notes
If you need any help, please email dev@ignite.apache.org or ask anу advice on http://asf.slack.com #ignite channel.