Closed bhalevy closed 4 years ago
asias
commented
4 years ago FYI, the commit a74a82d contains the patch below which avoids generating duplicate partition_end.
commit 401854dbafbe1e6be00c9060eff3827bd4786165
Author: Asias He <asias@scylladb.com>
Date: Tue Dec 31 17:50:45 2019 +0800
repair: Avoid duplicated partition_end writeThe validator says:
Unexpected partition key: previous {key: pk{00046b373034}, token:-9178076079136936125}, current {key: pk{00046b313930}, token:-9213292831655777591}, What does it actually mean? What was the problem?
denesb
commented
4 years ago The validator complains seeing a partition_start fragment, following another partition_start one.
bhalevy
commented
4 years ago The validator says:
Unexpected partition key: previous {key: pk{00046b373034}, token:-9178076079136936125}, current {key: pk{00046b313930}, token:-9213292831655777591},What does it actually mean? What was the problem?
It's misordered partition key, they should be monotonically increasing.
bhalevy
commented
4 years ago The validator complains seeing a
partition_startfragment, following anotherpartition_startone.
Where do you see indication for that? If it was so it would fail in https://github.com/scylladb/scylla/blob/a74a82d4d245d75311c19d0d8fba404d5d75937e/flat_mutation_reader.cc#L973 ... https://github.com/scylladb/scylla/blob/a74a82d4d245d75311c19d0d8fba404d5d75937e/flat_mutation_reader.cc#L992
But it fails in https://github.com/scylladb/scylla/blob/a74a82d4d245d75311c19d0d8fba404d5d75937e/flat_mutation_reader.cc#L944
denesb
commented
4 years ago Right, I misread the message.
slivne
commented
4 years ago @bhalevy does this happen only on master or on 3.2 as well ?
bhalevy
commented
4 years ago @bhalevy does this happen only on master or on 3.2 as well ?
I don't know. I'll try to reproduce with next-3.2
asias
commented
4 years ago @bhalevy and @denesb Just to be sure. What the validator really complains is: out of order partition keys, they should be monotonically increasing. Correct?
denesb
commented
4 years ago @asias yes -- in token order.
bhalevy
commented
4 years ago Same stack trace also in https://jenkins.scylladb.com/job/scylla-staging/job/fruch/job/dtest-byo-parallel/113/artifact/logs-release.2.9/1579681759929_repair_additional_test.RepairAdditionalTest.repair_disjoint_data_test/node3.log
inserts data in disjoint key ranges on different nodes with CL=ONE and the other nodes down, and eventually runs repair on node3
bhalevy
commented
4 years ago FWIW it didn't reproduce for me locally yet, neither with master nor with next-3.2 (both built in debug mode).
asias
commented
4 years ago Some analysis: The tests is different shards test. There are only 2 nodes n1 and n2 in the test. n1 has 2 shards and n2 has 3 shards. n2 is the repair master. Since there is only 2 nodes, repair mast won't sort the fragments from repair slaves at all, that is it will preserve the order whatever the flat mutation reader generates. The reorder should not happen at the repair layer. I suspect the reorder happens at multi-shard reader. The multi-shard reader is created on the repair follower (n1).
denesb
commented
4 years ago We need to bolt the validator onto the repair reader and reproduce. I can provide a patch.
denesb
commented
4 years ago From 9e3f9b8a960bfccbfa94d852d31791360423632b Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Botond=20D=C3=A9nes?= <bdenes@scylladb.com>
Date: Thu, 23 Jan 2020 11:18:20 +0200
Subject: [PATCH] repair/row_level: add validator to repair_reader
---
repair/row_level.cc | 17 +++++++++++++++--
1 file changed, 15 insertions(+), 2 deletions(-)
diff --git a/repair/row_level.cc b/repair/row_level.cc
index ffa04d4b6..bf6c31b23 100644
--- a/repair/row_level.cc
+++ b/repair/row_level.cc
@@ -373,6 +373,7 @@ using is_local_reader = bool_class<class is_local_reader_tag>;
std::optional<utils::phased_barrier::operation> _local_read_op;
// Local reader or multishard reader to read the range
flat_mutation_reader _reader;
+ mutation_fragment_stream_validator _validator;
// Current partition read from disk
lw_shared_ptr<const decorated_key_with_hash> _current_dk;
@@ -392,7 +393,8 @@ using is_local_reader = bool_class<class is_local_reader_tag>;
, _sharder(remote_partitioner, range, remote_shard)
, _seed(seed)
, _local_read_op(local_reader ? std::optional(cf.read_in_progress()) : std::nullopt)
- , _reader(make_reader(db, cf, local_partitioner, local_reader)) {
+ , _reader(make_reader(db, cf, local_partitioner, local_reader))
+ , _validator("repair_reader", *_schema, true) {
}
private:
@@ -416,7 +418,18 @@ using is_local_reader = bool_class<class is_local_reader_tag>;
public:
future<mutation_fragment_opt>
read_mutation_fragment() {
- return _reader(db::no_timeout);
+ return _reader(db::no_timeout).then([this] (mutation_fragment_opt mf_opt) {
+ if (!mf_opt) {
+ _validator.on_end_of_stream();
+ } else {
+ if (mf_opt->is_partition_start()) {
+ _validator(mf_opt->as_partition_start().key());
+ }
+ _validator(*mf_opt);
+ }
+
+ return mf_opt;
+ });
}
lw_shared_ptr<const decorated_key_with_hash>& get_current_dk() {
--
2.24.1
Also at https://github.com/denesb/scylla/tree/row-level-repair-reader-validation.
EDIT: patch fixed based on @bhalevy's comment, branch force pushed.
denesb
commented
4 years ago Even better I'll send a patch to the list to have conditionally enabled validation in repair, so we can have this running in jenkins. We want to do this for all read paths anyway (https://github.com/scylladb/scylla/issues/5615) so might as well do it for repair now.
asias
commented
4 years ago Update:
I tried to reproduce with Botond's read validator. I repeated the test (repair_disjoint_row_2nodes_diff_shard_count_test) 30 times and I could not reproduce. All passed.
commit c67f1b3011a4b1a7b9941928e986b63f7d7f8dc6
Author: Botond Dénes <bdenes@scylladb.com>
Date: Thu Jan 23 16:13:42 2020 +0200
row-level repair: allow validating the row level repair reader
Introduce the row level repair reader as a component for which validation
can be enabled, under the name `row_level_repair_reader`.Have you enabled the read validator? It is not enabled by default.
On Sun, 2020-02-02 at 18:47 -0800, Asias He wrote:
Update:
I tried to reproduce with Botond's read validator. I repeated the test (repair_disjoint_row_2nodes_diff_shard_count_test) 30 times and I could not reproduce.
All passed.
commit c67f1b3011a4b1a7b9941928e986b63f7d7f8dc6Author: Botond Dénes < bdenes@scylladb.com>Date: Thu Jan 23 16:13:42 2020 +0200 row-level repair: allow validating the row level repair reader Introduce the row level repair reader as a component for which validation can be enabled, under the name
row_level_repair_reader.— You are receiving this because you were assigned. Reply to this email directly, view it on GitHub, or unsubscribe.
[ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": " https://github.com/scylladb/scylla/issues/5627?email_source=notifications\u0026email_token=AAKTFWOIVPQMTP4FGQ6KGI3RA6AU7A5CNFSM4KKCJ35KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEKSKH4Q#issuecomment-581215218 ", "url": " https://github.com/scylladb/scylla/issues/5627?email_source=notifications\u0026email_token=AAKTFWOIVPQMTP4FGQ6KGI3RA6AU7A5CNFSM4KKCJ35KYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEKSKH4Q#issuecomment-581215218 ", "name": "View Issue" }, "description": "View this Issue on GitHub", "publisher": { "@type": "Organization", "name": "GitHub", "url": "https://github.com" } } ]
asias
commented
4 years ago Have you enabled the read validator? It is not enabled by default.
Unfortunately, no. Can you provide the cmd line option to enable? However, the 30 runs passed. So even if the validator was turned on, we would not catch anything.
asias
commented
4 years ago --mutation-fragment-stream-validation all:full sounds good?
denesb
commented
4 years ago I responded with email but it didn't appear for some reason:
--mutation-fragment-stream-validation 'row_level_repair_reader=all:full'
Yes, we should have seen the failure even without this additional validator.
asias
commented
4 years ago I responded with email but it didn't appear for some reason:
--mutation-fragment-stream-validation 'row_level_repair_reader=all:full'
4 FATAL: Exception during startup, aborting: std::invalid_argument (Invalid value for mutation_fragment_stream_validation: got validation_level full' which is not inside t he set of known validation levels {none, mutation_fragment_kind_monotonicity, partition_monotonicity, clustering_monotonicity, full})
asias
commented
4 years ago I tried a bunch of combinations for the validator options, all failed. We really need a concrete example in the help message.
Also,
--mutation-fragment-stream-validation 'row_level_repair_reader=all:full'
is confusing, because the component appeared twice. first row_level_repair_reader then all.
denesb
commented
4 years ago Try without the quotes:
--mutation-fragment-stream-validation row_level_repair_reader=all:fullWhy are those not stripped by the shell?
all refers to the tables for which validation is enabled.
asias
commented
4 years ago Try without the quotes:
--mutation-fragment-stream-validation row_level_repair_reader=all:fullWhy are those not stripped by the shell?
It works now. Running another set of tests with it enabled.
allrefers to the tables for which validation is enabled.
OK.
asias
commented
4 years ago Another 30 runs passed with reader validator enabled.
slivne
commented
4 years ago @bhalevy have you been able to reproduce with dtest on jenkins ?
bhalevy
commented
4 years ago @bhalevy have you been able to reproduce with dtest on jenkins ?
No, I don't think it was reproduced yet on jenkins.
slivne
commented
4 years ago @bhalevy did his reproduce ?
bhalevy
commented
4 years ago @slivne This wasn't reproduced anywhere (jenkins / SCT / locally) AFAIK.
slivne
commented
4 years ago I am closing this issue - as we are not able to hit it, if we will we can open a new issue
Scylla version (or git commit hash): a74a82d4d2 Seen in https://jenkins.scylladb.com/view/master/job/scylla-master/job/dtest-debug-random/50/artifact/logs-debug.2/orphaned/1579603734_repair_additional_test.RepairAdditionalTest.repair_disjoint_row_2nodes_diff_shard_count_test/node2.log
Decoded: