Mas i1801 2iqueryperf by martinsumner · Pull Request #1802 · basho/riak_kv

martinsumner · 2021-10-27T14:50:42Z

Adds stats to secondary index queries. Adds to riak stats for each vnode fold, and adds an optional log to the riak_kv_index_fsm.

The optional log can be enabled by setting riak_kv.log_index_fsm to true (or enabling this via riak.conf).

Dependencies are updated to included the revised riak_core_coverage_plan:initiate_plan/5 function which offers significant performance advantages for larger ring_sizes.

martinsumner · 2021-10-27T14:52:48Z

basho/riak_test#1359

Rather than just option to log, stats will be updated as with get_fsm and put_fsm. Stats include response time histogram, but also a histogram of result counts. This should be useful for identifying when there are issues related to very large result sets being returned, and establish any non-linear relationship between query time and result counts.

the monitoring of these times has switched to riak_core e.g. worker_af4_pool_queuetime_mean" worker_af4_pool_queuetime_100 worker_af4_pool_worktime_mean worker_af4_pool_worktime_100 worker_vnode_pool_queuetime_mean worker_vnode_pool_queuetime_100 worker_vnode_pool_worktime_mean worker_vnode_pool_worktime_100

Previously only possible to set this via advanced.config. The default has been reduced, as with a large pool, it is possible that context switching between a vast number of concurrent folds could be slower overall than simply queueing some folds to allow others to complete. Even on volume tests with 1% of transactions using 2i queries, a pool size of 4 is never exhausted.

ThomasArts · 2021-11-09T09:26:06Z


 {deps, [
-    {riak_core, {git, "https://github.com/basho/riak_core.git", {tag, "riak_kv-3.0.8"}}},
+    {riak_core, {git, "https://github.com/basho/riak_core.git", {branch, "mas-i1801-monitorworkerq"}}},


work needed before merge (I mean, don't forget to reset to right branches)

ThomasArts · 2021-11-09T09:36:30Z

-                  end,
+    TotalResults = length(LastResults) + ResultsSent,
+    DownTheWire =
+        case TotalResults > MaxResults of


We don't need TotalResults. The function lists:sublist takes care of this, taking only the MaxResult - ResultSent is sufficient, making the code substantially easier:

Suggested change

case TotalResults > MaxResults of

MaxResults, it turns out can be the atom all as well as an integer(), and this relies on the fact any integer() > atom() is false.

So if we just have lists:sublist we get badarith.

But it is a bit worse, as it is also possible to construct a query whereby MaxResults is undefined, but a page-sort query is still requested. undefined isn't expected by dialyzer.

Going to make the case of all vs integer() more specific, and ensure that not integer() is always all

ThomasArts · 2021-11-09T09:36:53Z

-                      false ->
-                          LastResults
-                  end,
+    TotalResults = length(LastResults) + ResultsSent,


Suggested change

TotalResults = length(LastResults) + ResultsSent,

ThomasArts · 2021-11-09T09:37:20Z

+    TotalResults = length(LastResults) + ResultsSent,
+    DownTheWire =
+        case TotalResults > MaxResults of
+            true ->


Suggested change

true ->

ThomasArts · 2021-11-09T09:37:42Z

+    DownTheWire =
+        case TotalResults > MaxResults of
+            true ->
+                lists:sublist(LastResults, MaxResults - ResultsSent);


Suggested change

lists:sublist(LastResults, MaxResults - ResultsSent);

lists:sublist(LastResults, MaxResults - ResultsSent),

ThomasArts · 2021-11-09T09:37:55Z

+        case TotalResults > MaxResults of
+            true ->
+                lists:sublist(LastResults, MaxResults - ResultsSent);
+            false ->


Suggested change

false ->

ThomasArts · 2021-11-09T09:38:06Z

+            true ->
+                lists:sublist(LastResults, MaxResults - ResultsSent);
+            false ->
+                LastResults


Suggested change

LastResults

ThomasArts · 2021-11-09T09:38:17Z

+                lists:sublist(LastResults, MaxResults - ResultsSent);
+            false ->
+                LastResults
+        end,


Suggested change

end,

ThomasArts · 2021-11-09T09:44:21Z

    ok = do_stages([P, ?APP, node, puts, Type, time], Stages),
    do_put_bucket(PerBucket, {Bucket, Microsecs, Stages, Type});
+do_update({index_fsm_time, Microsecs, ResultCount}) ->
+    P = ?PFX,


Correct, but I like 3 copies of ?PFX in arguments below better... Renaming a macro makes it not as easy to see that a macro is used.

In line 272 below the macro is also used, so more in line with that code

The convention in the module is that if ?PFX is required more than once in a function, it is called then assigned rather than repeatedly called. It might be a performance thing (this a very active piece of code, ignoring excess calls to get_env), or perhaps a consistency thing (so that the prefix can't change across a set of updates.

Without knowing why this is the convention, I don't wish to change it.

if N > length of list, whole list sent in lists:sublist

mas-i1801-monitorworkerq now merged in

…eryperf

martinsumner · 2021-11-11T09:51:34Z

Full list of stats added by this PR:

index_fsm_complete

this increments every time a 2i query complete (there is already a counter for every time one is started)

index_fsm_results_mean
index_fsm_results_median
index_fsm_results_95
index_fsm_results_99
index_fsm_results_100

a histogram of the numbers of results seen in 2i queries on this node

index_fsm_time_mean
index_fsm_time_median,
index_fsm_time_95,
index_fsm_time_99
index_fsm_time_100

a histogram of the time taken for 2i queries on this node

worker_vnode_pool_queuetime_mean"
worker_vnode_pool_queuetime_100
worker_vnode_pool_worktime_mean
worker_vnode_pool_worktime_100

the max and mean time spent on each vnode running queries, both queueing to be run and actually running

worker_af1_pool_queuetime_mean"
worker_af1_pool_queuetime_100
worker_af1_pool_worktime_mean
worker_af1_pool_worktime_100
..... (i.e. repeated for af2_pool etc)

as above, but for each worker pool which can run queries (i.e. those for aae_folds, rebuilds etc)

martinsumner added 4 commits October 20, 2021 13:20

Add timings stats for 2i queries

db4ff30

Fix spacing issue in log

753ecff

Update rebar.config

9249446

Update rebar.config

fd620c8

martinsumner added 6 commits November 1, 2021 12:01

Count results when not paginated

e63dfc1

Switch to monitoring branch

4f8b4f9

Remove stat references

e105a31

martinsumner requested a review from ThomasArts November 8, 2021 14:19

ThomasArts reviewed Nov 9, 2021

View reviewed changes

ThomasArts approved these changes Nov 9, 2021

View reviewed changes

martinsumner added 4 commits November 9, 2021 10:39

Remove unnecessary case

697fc95

if N > length of list, whole list sent in lists:sublist

Force MaxResults to be 'all' or integer() at init

9b089ce

Switch core branch

bbdfa48

mas-i1801-monitorworkerq now merged in

Merge remote-tracking branch 'origin/develop-3.0' into mas-i1801-2iqu…

a1750b0

…eryperf

martinsumner merged commit 44849c1 into develop-3.0 Nov 11, 2021

martinsumner deleted the mas-i1801-2iqueryperf branch November 11, 2021 10:34

martinsumner mentioned this pull request Nov 12, 2021

2i query time and ring_size #1801

Closed

JMercerGit mentioned this pull request Jan 19, 2022

update stats documentation to reflect new stats in 3.0.9 TI-Tokyo/riak-docs-fork#79

Open

	lists:sublist(LastResults, MaxResults - ResultsSent);
	lists:sublist(LastResults, MaxResults - ResultsSent),

Uh oh!

Conversation

martinsumner commented Oct 27, 2021

Uh oh!

martinsumner commented Oct 27, 2021

Uh oh!

ThomasArts Nov 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

martinsumner commented Nov 11, 2021

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

ThomasArts Nov 9, 2021 •

edited

Loading