Commit cb64d5b
committed
[yugabyte#8485] Refactor split op info to generic operation blocking mechanism
Summary:
This diff removes split op id from replica state. This split op id was being used for the various purposes:
1) Block adding new write (and some other) operations to the Raft log after the split operation started to replicate.
2) Store data for child tablet ids.
3) Retain split op id in logs.
It was changed to the following:
1) Introduced a generic mechanism to dynamically reject operations.
2) Store split op id and child tablet ids in the tablet metadata (superblock). This is a persistent format change.
3) Use log anchor registry.
Also moved lease check higher by call stack and do it only for the last operation in batch.
Removed an unused index of a multi index container (on the op id) from `RunningRetryableRequests`.
Changes to duplicate request error handling
===========================================
RetryableRequests is a subsystem we use for filtering out duplicate requests that could have been submitted during retries. All operations submitted to Raft are also registered with RetryableRequests. For a duplicate requests, we handle failure as follows. Either the Register function notifies the duplicate operation that it failed immediately (e.g. in case the original operation is known to have failed), or it waits for the original request to finish replicating and then notifies the duplicate operation of replication failure. After that the operation is removed from MvccManager and is rolled back in ReplicaState using its op id.
Also, when we add operations to Raft consensus, we do it in batches, and if at least one operation fails to get added, we fail the entire batch.
There are two issues with the interaction between these mechanisms.
1. A duplicate request might be waiting for the original operation to finish replicating, and therefore the duplicate request is not deleted from MvccManager right away. As a result, an operation with the same op id could be added to MvccManager. Prior to the recent commit 1aece37 (D11371), this was not an issue, because MvccManager only contained hybrid time (but no op ids) and the duplicate operation's hybrid time was greater than the original operation's hybrid time, so the read path was not affected (and we allow handling aborts in any order).
2. If the operation that fails is not the first operation in its batch (which would mean we've already called RetryableRequests::Register on some earlier operations in the batch), we call ReplicationFinished for all preceding operations in the batch, and no one would notify RetryableRequests about the completion of those preceding operations, so they would stay in RetryableRequests indefinitely.
The above issues became more frequent with this diff because we now check the lease after adding all operations. In the most frequent case when we have a valid leader lease, with the new behavior we will spend less time checking lease, and would not be reading the system clock repeatedly. However, the new logic might be slightly suboptimal in the rare case when the lease has expired.
The second scenario above has become more frequent with the changes in this diff (but before the fix described below) because with the lease check happening after adding all operations in the batch, we would have added all the operations to RetryableRequests. Prior to this diff, for operations to get stuck in RetryableRequests in this way, the lease check would need to succeed e.g. for the first operation so it would get added to RetryableRequests, but then it would need to fail for the second operation, and this combination of circumstances is very rare.
To fix the above issue, in this diff we handle 3 different cases:
1. Rounds that were rejected by RetryableRequests. We should not call ReplicationFinished for them as it would have been already called by Register.
2. Rounds that were registered with RetryableRequests. We should call NotifyReplicationFinishedUnlocked for them, which also notifies RetryableRequests of the operation failure.
3. Rounds that were not registered with retryable requests. We should call ReplicationFinished directly for them.
Test Plan: Jenkins
Reviewers: timur, mbautin
Reviewed By: mbautin
Subscribers: mbautin, bogdan, ybase
Differential Revision: https://phabricator.dev.yugabyte.com/D115901 parent 04293a4 commit cb64d5b
File tree
72 files changed
+972
-1051
lines changed- src/yb
- consensus
- integration-tests
- master
- tablet
- operations
- tserver
- util
Some content is hidden
Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.
72 files changed
+972
-1051
lines changed| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
128 | 128 | | |
129 | 129 | | |
130 | 130 | | |
131 | | - | |
| 131 | + | |
132 | 132 | | |
133 | 133 | | |
134 | 134 | | |
135 | 135 | | |
136 | 136 | | |
137 | | - | |
138 | | - | |
139 | 137 | | |
140 | 138 | | |
141 | 139 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
62 | 62 | | |
63 | 63 | | |
64 | 64 | | |
65 | | - | |
66 | | - | |
67 | | - | |
68 | | - | |
69 | 65 | | |
70 | 66 | | |
71 | 67 | | |
| |||
101 | 97 | | |
102 | 98 | | |
103 | 99 | | |
104 | | - | |
105 | | - | |
106 | | - | |
107 | | - | |
108 | | - | |
109 | | - | |
110 | 100 | | |
111 | 101 | | |
112 | 102 | | |
| |||
240 | 230 | | |
241 | 231 | | |
242 | 232 | | |
243 | | - | |
| 233 | + | |
244 | 234 | | |
245 | 235 | | |
246 | 236 | | |
| |||
336 | 326 | | |
337 | 327 | | |
338 | 328 | | |
339 | | - | |
340 | | - | |
341 | | - | |
342 | | - | |
343 | | - | |
344 | | - | |
345 | | - | |
346 | | - | |
347 | 329 | | |
348 | 330 | | |
349 | 331 | | |
| |||
520 | 502 | | |
521 | 503 | | |
522 | 504 | | |
523 | | - | |
524 | | - | |
| 505 | + | |
| 506 | + | |
525 | 507 | | |
526 | 508 | | |
527 | 509 | | |
| |||
549 | 531 | | |
550 | 532 | | |
551 | 533 | | |
552 | | - | |
553 | 534 | | |
554 | 535 | | |
555 | 536 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
542 | 542 | | |
543 | 543 | | |
544 | 544 | | |
545 | | - | |
546 | 545 | | |
547 | 546 | | |
548 | 547 | | |
| |||
554 | 553 | | |
555 | 554 | | |
556 | 555 | | |
| 556 | + | |
| 557 | + | |
557 | 558 | | |
558 | 559 | | |
559 | 560 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
17 | 17 | | |
18 | 18 | | |
19 | 19 | | |
| 20 | + | |
20 | 21 | | |
21 | 22 | | |
22 | 23 | | |
| |||
69 | 70 | | |
70 | 71 | | |
71 | 72 | | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
72 | 77 | | |
73 | 78 | | |
74 | 79 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
19 | 19 | | |
20 | 20 | | |
21 | 21 | | |
22 | | - | |
23 | | - | |
24 | | - | |
25 | | - | |
26 | | - | |
27 | | - | |
28 | | - | |
29 | 22 | | |
30 | 23 | | |
31 | 24 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
62 | 62 | | |
63 | 63 | | |
64 | 64 | | |
65 | | - | |
66 | | - | |
67 | | - | |
68 | | - | |
69 | 65 | | |
70 | 66 | | |
71 | 67 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
147 | 147 | | |
148 | 148 | | |
149 | 149 | | |
150 | | - | |
151 | 150 | | |
152 | 151 | | |
153 | 152 | | |
| |||
1604 | 1603 | | |
1605 | 1604 | | |
1606 | 1605 | | |
| 1606 | + | |
| 1607 | + | |
| 1608 | + | |
| 1609 | + | |
| 1610 | + | |
1607 | 1611 | | |
1608 | 1612 | | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
63 | 63 | | |
64 | 64 | | |
65 | 65 | | |
66 | | - | |
67 | | - | |
68 | | - | |
69 | | - | |
70 | | - | |
71 | 66 | | |
72 | 67 | | |
73 | 68 | | |
| |||
378 | 373 | | |
379 | 374 | | |
380 | 375 | | |
| 376 | + | |
| 377 | + | |
381 | 378 | | |
382 | 379 | | |
383 | 380 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
36 | 36 | | |
37 | 37 | | |
38 | 38 | | |
39 | | - | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
40 | 45 | | |
41 | 46 | | |
42 | 47 | | |
43 | 48 | | |
44 | 49 | | |
45 | 50 | | |
46 | 51 | | |
47 | | - | |
48 | | - | |
49 | | - | |
50 | | - | |
51 | | - | |
52 | 52 | | |
53 | 53 | | |
54 | 54 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
642 | 642 | | |
643 | 643 | | |
644 | 644 | | |
645 | | - | |
| 645 | + | |
646 | 646 | | |
647 | 647 | | |
648 | 648 | | |
| |||
669 | 669 | | |
670 | 670 | | |
671 | 671 | | |
672 | | - | |
673 | 672 | | |
674 | 673 | | |
675 | 674 | | |
| |||
715 | 714 | | |
716 | 715 | | |
717 | 716 | | |
718 | | - | |
| 717 | + | |
719 | 718 | | |
720 | 719 | | |
721 | 720 | | |
| |||
1052 | 1051 | | |
1053 | 1052 | | |
1054 | 1053 | | |
1055 | | - | |
| 1054 | + | |
1056 | 1055 | | |
1057 | 1056 | | |
1058 | 1057 | | |
| |||
0 commit comments