Mas i1815 autocheck by martinsumner · Pull Request #1816 · basho/riak_kv

martinsumner · 2022-03-09T22:25:12Z

To simplify the configuration, rather than have the operator select all_check day_check range_check etc, there is now a default strategy of auto_check which tries to do a sensible thing:

do range_check when a range is set, otherwise
do all_check if it is out of hours (in the all_check window), otherwise
do day_check

Some stats added to help with monitoring, the detail is still also in the console logs.

See #1815

martinsumner · 2022-03-09T22:25:20Z

basho/riak_test#1364

martinsumner · 2022-03-09T22:26:32Z

Awaiting documentation and schema changes

s2hc-johan

Assuming L249-L261 in docs/NextGenREPL.md describes defaults there are inconsistencies with what is defined in the riak_kv.schema.

Also the default window of 0-23 if window is configured isn’t in the documentation either I think.

As a summary of the PR I don’t see any code change; but added docs, configuration and tests.

s2hc-johan · 2022-03-23T10:50:48Z


 If a previous check is still running when the allocated scheduled time for the next check on this node occurs, the following check will not be run.  Checks are skipped if the node falls behind schedule.

+The `ttaaefs_rangecheck` and `ttaaefs_autocheck` are smart checks, the adapt to previous results.  If the previous sync event was successful, and the `ttaaefs_rangecheck` discovers a discrepancy, it will assume that the discrepancy must be for data with a last modified date since the last successful sync.  If the previous sync showed a discrepancy, it will assume the discrepancy in the nexy sync event is in the "same data" (either bucket, or last modified date range) as the previous check.  If the discrepancy was largely unresolvable (i.e. because the sink was ahead in most cases), the `ttaaefs_rangecheck` will not a different type of check discovers that this has changed.


the adapt to ...

Shouldn't that be

they adapt to ...

The last sentance ends a bit strange

... will not a different type of check discovers ...

s2hc-johan · 2022-03-23T10:55:29Z


+The `ttaaefs_rangecheck` and `ttaaefs_autocheck` are smart checks, the adapt to previous results.  If the previous sync event was successful, and the `ttaaefs_rangecheck` discovers a discrepancy, it will assume that the discrepancy must be for data with a last modified date since the last successful sync.  If the previous sync showed a discrepancy, it will assume the discrepancy in the nexy sync event is in the "same data" (either bucket, or last modified date range) as the previous check.  If the discrepancy was largely unresolvable (i.e. because the sink was ahead in most cases), the `ttaaefs_rangecheck` will not a different type of check discovers that this has changed.
+
+The `ttaaefs_autocheck` will morph into another check when the check is called.  It will morph into a `ttaaefs_rangecheck` if a range of resolvable discrepancies had been discovered by the previous sync event.  Otherwise it will either run a `ttaaefs_allcheck` or `ttaaefs_daycheck` depending on whether the time is inside or outside of the `ttaaefs_allcheck.window`.


For my curiosity, the window is so that it will do day check during “office hours” and a all check otherwise?

Yes. The configuration is based on the bitcask merge window, where you can set a window out of hours for merges to happen. In this case we can set a window out of hours for all_checks to work in. So if you have delta in historic data, it will be resolved out of hours.

s2hc-johan · 2022-03-23T11:02:49Z

 {mapping, "ttaaefs_allcheck", "riak_kv.ttaaefs_allcheck", [
  {datatype, integer},
-  {default, 24}
+  {default, 0}


I think that the documentation refers to 24 still, don’t know which is correct but they should probably be conssitent

s2hc-johan · 2022-03-23T11:04:07Z

+
+%% @doc How many times per 24hour period should the an auto_check be run.  The
+%% auto_check is intended to be an adaptive check, in that it will:
+%% - use a range_check if the revious check has identified a range;


revious -> previous

s2hc-johan · 2022-03-23T11:07:02Z


+%% @doc Lets you specify when during the day auto_check may run all_check
+%% rather than the less expensive day_check, when no range has been identified.
+%% This constrains fixing entropy between cluster sof no recently-modified data


should it be “cluster sof” -> “clusters of”

s2hc-johan · 2022-03-23T11:12:14Z

    ok = exometer:update([?PFX, ?APP, node, puts, ngrrepl_object], 1);
 do_update(ngrrepl_error) ->
    ok = exometer:update([?PFX, ?APP, node, puts, ngrrepl_error], 1);
+do_update({ttaaefs, all_check}) ->


Not looking deep, do we have tests that cover functionality or should I script to look for spelling mistakes or something?

The riak_test test checks the stats . If you look at the get_stats function and its use in https://github.com/basho/riak_test/blob/mas-i1804-peerdiscovery/tests/nextgenrepl_rtq_peerdiscovery.erl.

There is also a verify_riak_stats test which gets updated, which just checks that all things are named correctly, and there are no unexpected entries: https://github.com/basho/riak_test/blob/mas-i1804-peerdiscovery/tests/verify_riak_stats.erl

If this looks like it is covering everything OK, then we're fine. However, if anything jumps out as looking weird, flag it, and I will add some coverage to the test.

s2hc-johan · 2022-03-23T11:16:10Z

      [{value, leveldb_read_block_error}]},
     {tictacaae_controller_queue, histogram, [], [{mean, tictacaae_queue_microsec_mean},
                                                    {max, tictacaae_queue_microsec__max}]},
+


don’t really know what to look for here

s2hc-johan · 2022-03-23T11:18:14Z

 {mapping, "ttaaefs_rangecheck", "riak_kv.ttaaefs_rangecheck", [
  {datatype, integer},
-  {default, 0}
+  {default, 12}


default sync with docs

s2hc-johan · 2022-03-23T11:18:32Z

+%% that nval.
+{mapping, "ttaaefs_autocheck", "riak_kv.ttaaefs_autocheck", [
+  {datatype, integer},
+  {default, 12}


default sync with docs

Previously the documentation had stated that smart checks were only relevant to `bucket` or `type` scope. In fact they can make the `all` scope more efficient in clock comparison.

Removes need to split default config between range_check and auto_check

May prompt range_check without range

full-sync can now do bi-directional repairs!! The auto_check is adapted further, so that there is no reason to split the default configuration between auto_check and range_check. The auto_check method should almost always be the right one.

s2hc-johan · 2022-04-01T15:36:50Z

+`ttaaefs_autocheck = 12`

-If `bucket` or `type` is the scope for this node, then two additional sync counts can be configured.  The `hourcheck` and `daycheck` will be configured to sync only for objects modified in the past hour or day (as determined by the objects last_modified metadata).  When running a full-sync check restricted to a bucket or type, this is less efficient than running a check for an entire n_val as cached trees may not be used.  Running a `ttaaefs_allcheck` may take o(hour) on a large bucket, whereas `ttaaefs_daycheck` may be o(minute) and `ttaaefs_hourcheck` may be o(second).  So it would be preferable to frequently check recent modifications have synced correctly, and less frequently check that older modifications have synchronised.
+There are four additional sync type counts which can be configured, and these are of particular importance if `bucket` or `type` is the scope of full-sync for this node.  There are two stages to the full-sync - the tree comparison, and then the clock comparison (comparing the objects ona  subset of the damaged portion of the tree).  If the scope of the full-sync is `all` the data, then the tree comparison is very low cost (based on cached trees).  If the scope of the full-sync is `bucket` or `type` then the cost of the tree comparison increases with the number of keys in the bucket.  The clock comparison also with the size of the database (of the buckets covered by the scope).


... ona subset of ...
should probably be
... on a subset of ...

The encoding of vclocks differs between http and pb due to the need to base64 encode for http. Case clauses change for auto_check, as otherwise may have requested a range_check without a range or previous success, when drop_next_auto_check() was true, even when the peer queue is not diabled.

Also simplify the determine_next_action logic by making the target for making a range more obvious/predictable

The coordination of syncs between clusters caused performance spikes in testing. A simple (and optional) way round this is the configuration of cluster slice numbers. Also, this change simplifies whether or not a range will be defined. As auto_check used by default - other checks are just a "finder", and so can be accelerated by using less max_results. Even with a relatively small sample size, significant "clipping" of the boundary is unlikely (e.g. only 3% chance the lmd range will be clipped by more than 10%).

Schedule spacing was dependent on start-up time being consistent, so now instead we use the beginning of the next hour as the anchor (following startup).

Initial schedule time has not been as expected. Increase logging

Cluster could be in a change state at startup so relying on node counts etc at this point for next 24 hours seems too unstable. Reset when ever node info changes, in case it wasn't right at first pass.

In wild seen {4, 3, 1} returned when the node hasn't started after the initial timeout. this node will always eventually be up, so always include this node in the UpNodes and avoid any situation where this node is n+1 of n.

If the aae_trees are broken on multiple nodes - i.e. the same segment represented incorrectly in the same way on all 3 nodes, then internal anti-entropy cannot repair this. This can lead to a situation where inter-cluster anti-entropy can never resolve. This can be resolved by enforcing the rebuild of the trees within the aae_fold fetch clocks (although this has an efficiency overhead). Rather than rely on operator intervention to trigger in this case, this code change will spot {clock_compare,0} on an all_check and set this parameter on the node. Eventually, if most nodes have peers, then this will be resolved a sit is turned on node by node. Once a sync is successful, it will be turned back off.

Otherwise an allocation may have been prompted. Now on timeout there will be no allocations, and a new set of no_checks. In case a check was already in flight, we check pause on handle_info of work_item as well

This reverts commit 4e3d5fb.

Merge in changes from Riak 3.0.10. Includes PRs: - #1809 - #1812 - #1814 - #1816 - #1829 - #1830

To simplify the configuration, rather than have the operator select all_check day_check range_check etc, there is now a default strategy of auto_check which tries to do a sensible thing: do range_check when a range is set, otherwise do all_check if it is out of hours (in the all_check window), otherwise do day_check Some stats added to help with monitoring, the detail is still also in the console logs. See basho#1815 # Conflicts: # src/riak_kv_stat.erl # src/riak_kv_ttaaefs_manager.erl

martinsumner added 2 commits March 9, 2022 20:24

Add auto-check

395f35c

Add stats for ttaae full-sync

d1c7102

martinsumner mentioned this pull request Mar 9, 2022

Replication auto_check for nextgenrepl full-sync #1815

Closed

martinsumner added 3 commits March 11, 2022 10:03

add configuration for auto_check and all_check window

c92b746

Fix translation to use riak_kv as application

3ad7f3e

Change full-sync defaults to use autocheck and rangecheck

b5292aa

s2hc-johan reviewed Mar 23, 2022

View reviewed changes

martinsumner added 5 commits March 30, 2022 12:42

Documentation updates following review

5839fed

Previously the documentation had stated that smart checks were only relevant to `bucket` or `type` scope. In fact they can make the `all` scope more efficient in clock comparison.

Resolve git reference

fd2a5f7

Add skip to auto_check

9d0ad30

Removes need to split default config between range_check and auto_check

Don't use previous success

a0074ee

May prompt range_check without range

Update auto-check

df87ee6

full-sync can now do bi-directional repairs!! The auto_check is adapted further, so that there is no reason to split the default configuration between auto_check and range_check. The auto_check method should almost always be the right one.

s2hc-johan reviewed Apr 1, 2022

View reviewed changes

martinsumner added 14 commits April 2, 2022 00:55

Use pb by default for replication

2d4d1b9

Also simplify the determine_next_action logic by making the target for making a range more obvious/predictable

Doc update - new defaults

9590274

Resolve schedule spacing

d094163

Schedule spacing was dependent on start-up time being consistent, so now instead we use the beginning of the next hour as the anchor (following startup).

Increase wait and info at startup

5666384

Initial schedule time has not been as expected. Increase logging

Add resetting of schedule

e7ff1f7

Cluster could be in a change state at startup so relying on node counts etc at this point for next 24 hours seems too unstable. Reset when ever node info changes, in case it wasn't right at first pass.

Remove redundant log

3978604

Assume node must eventually come up

b3da19f

In wild seen {4, 3, 1} returned when the node hasn't started after the initial timeout. this node will always eventually be up, so always include this node in the UpNodes and avoid any situation where this node is n+1 of n.

Pause/resume requires allocations to be clear

2080178

Otherwise an allocation may have been prompted. Now on timeout there will be no allocations, and a new set of no_checks. In case a check was already in flight, we check pause on handle_info of work_item as well

Manage multiple loops

4e3d5fb

Revert "Manage multiple loops"

e76dd62

This reverts commit 4e3d5fb.

Merge branch 'develop-3.0' into mas-i1815-autocheck

de03178

martinsumner merged commit 33547a4 into develop-3.0 May 12, 2022

martinsumner deleted the mas-i1815-autocheck branch May 12, 2022 15:38

martinsumner restored the mas-i1815-autocheck branch May 13, 2022 08:48

martinsumner added a commit that referenced this pull request May 31, 2022

Mas 310 merge (#1832)

a632efe

Merge in changes from Riak 3.0.10. Includes PRs: - #1809 - #1812 - #1814 - #1816 - #1829 - #1830


		If a previous check is still running when the allocated scheduled time for the next check on this node occurs, the following check will not be run. Checks are skipped if the node falls behind schedule.

		The `ttaaefs_rangecheck` and `ttaaefs_autocheck` are smart checks, the adapt to previous results. If the previous sync event was successful, and the `ttaaefs_rangecheck` discovers a discrepancy, it will assume that the discrepancy must be for data with a last modified date since the last successful sync. If the previous sync showed a discrepancy, it will assume the discrepancy in the nexy sync event is in the "same data" (either bucket, or last modified date range) as the previous check. If the discrepancy was largely unresolvable (i.e. because the sink was ahead in most cases), the `ttaaefs_rangecheck` will not a different type of check discovers that this has changed.


		The `ttaaefs_rangecheck` and `ttaaefs_autocheck` are smart checks, the adapt to previous results. If the previous sync event was successful, and the `ttaaefs_rangecheck` discovers a discrepancy, it will assume that the discrepancy must be for data with a last modified date since the last successful sync. If the previous sync showed a discrepancy, it will assume the discrepancy in the nexy sync event is in the "same data" (either bucket, or last modified date range) as the previous check. If the discrepancy was largely unresolvable (i.e. because the sink was ahead in most cases), the `ttaaefs_rangecheck` will not a different type of check discovers that this has changed.

		The `ttaaefs_autocheck` will morph into another check when the check is called. It will morph into a `ttaaefs_rangecheck` if a range of resolvable discrepancies had been discovered by the previous sync event. Otherwise it will either run a `ttaaefs_allcheck` or `ttaaefs_daycheck` depending on whether the time is inside or outside of the `ttaaefs_allcheck.window`.

Uh oh!

Conversation

martinsumner commented Mar 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

martinsumner commented Mar 9, 2022

Uh oh!

martinsumner commented Mar 9, 2022

Uh oh!

s2hc-johan left a comment

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!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

martinsumner commented Mar 9, 2022 •

edited

Loading