Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[docdb] Regular DB special records could affect the correctness of a middle key calculation for split #10163

Closed
ttyusupov opened this issue Oct 1, 2021 · 0 comments
Closed

[docdb] Regular DB special records could affect the correctness of a middle key calculation for split #10163

ttyusupov opened this issue Oct 1, 2021 · 0 comments
Assignees
@arybochkin
Labels
area/docdb YugabyteDB core features kind/bug This issue is a bug priority/high High Priority
Projects
YBase features Tablet splitting

Comments

@ttyusupov
Copy link
Contributor

ttyusupov commented Oct 1, 2021 •
edited

The presence of special records inside regular RocksDB could affect the correctness of a middle key calculation. If we have more special records than regular records in the SST file - we might get a middle key that belongs to the special records key range.

From: @spolitov :

we have one apply record per 100k entries of large transaction. moreover it is present only during apply, so could be removed by compaction.

So, just throwing an error, in that case, should work and most likely is already happening, but we need to recheck and test this.
@ttyusupov ttyusupov added kind/bug This issue is a bug priority/high High Priority labels Oct 1, 2021
@ttyusupov ttyusupov added this to Next in Tablet splitting via automation Oct 1, 2021
@ttyusupov ttyusupov added this to Backlog in YBase features via automation Oct 1, 2021
@ttyusupov ttyusupov added the area/docdb YugabyteDB core features label Oct 1, 2021
@rthallamko3 rthallamko3 changed the title Regular DB special records could affect the correctness of a middle key calculation for split [docdb] Regular DB special records could affect the correctness of a middle key calculation for split Oct 18, 2021
@bmatican bmatican mentioned this issue Nov 6, 2021
Closed
robertsami added a commit that referenced this issue Nov 11, 2021
@robertsami
[#8242] Enable automatic tablet splitting by default
c8509d1 
Summary:
This diff enables automatic tablet splitting by default in 2.11.0

In addition, we set `txn_max_apply_batch_records` to its max value to effectively turn off this feature. We do this to avoid interaction with a known bug whereby ApplyTransactionState records adversely affect split key calculation. See #10163

Test Plan:
Ran itest with no test failures

Jenkins: rebase: 2.11.0
Jenkins: hot

Reviewers: timur, rao, bogdan

Reviewed By: bogdan

Subscribers: jenkins-bot, ybase

Differential Revision: https://phabricator.dev.yugabyte.com/D13828
robertsami added a commit that referenced this issue Nov 24, 2021
@robertsami
[#10163] dst - Disable large transaction batch apply until it does no…
f0b6164 
…t affect tablet splitting

Summary:
We currently have a bug whereby ApplyTransactionState records can throw off our split key
calculation. This revision effectively turns off large transaction batching entirely. We should
re-enable it once this bug is fixed.

Test Plan: Jenkins

Reviewers: bogdan, timur, rthallam

Reviewed By: rthallam

Subscribers: ybase

Differential Revision: https://phabricator.dev.yugabyte.com/D13906
@bmatican bmatican assigned arybochkin and unassigned rthallamko3 Dec 8, 2021
@bmatican bmatican removed the priority/high High Priority label Dec 8, 2021
@bmatican bmatican moved this from Next to In progress in Tablet splitting Dec 8, 2021
@bmatican bmatican moved this from In progress to Next in Tablet splitting Dec 8, 2021
@arybochkin arybochkin moved this from Next to In progress in Tablet splitting Dec 17, 2021
@arybochkin arybochkin moved this from Backlog to In progress in YBase features Dec 17, 2021
@ttyusupov ttyusupov mentioned this issue Jan 20, 2022
Open
@bmatican bmatican added the priority/high High Priority label Feb 9, 2022
arybochkin added a commit that referenced this issue Feb 18, 2022
@arybochkin
[#10163] docdb: Regular DB special records could affect the correctne…
fb24aea 
…ss of a middle key calculation for split

Summary:
The presence of special records inside regular RocksDB could affect the correctness of middle key
calculation. Having a special key as the split key will affect the correctness of tablet spitting.
Returning an error status is enough in that case as this will delay the tablet spitting (tablet
will receive more records and this will lead to a different middle key being picked). Most likely
this error will never be hit as the number of special records is negligibly small and they sort
before any user record. Special keys are `(kTransactionApplyState, 7)` for regular DB,
`(kExternalTransactionId, 8)`and `(kTransactionId, 'x')` for intents-db.

This fix updates `Tablet::GetEncodedMiddleSplitKey()` to throw an error when a special key is
selected as the middle key (and some other cases where a more sensible error is needed).
Corresponding tests are also added both for hash and range partitioning as DocKeys are generated in
a slightly different way depending on partitioning. Also several test routines were updated
to provide handling with respect to current partitioning.

Test Plan:
ybd --cxx-test integration-tests_tablet-split-itest --gtest_filter "*TabletSplitSystemRecordsITest.GetSplitKey*"
ybd --cxx-test integration-tests_tablet-split-itest
ybd --cxx-test tablet_tablet-split-test
ybd --cxx-test client_ql-tablet-test --gtest_filter QLTabletTest.GetMiddleKey

Reviewers: timur, rsami

Reviewed By: rsami

Subscribers: mbautin, ybase

Differential Revision: https://phabricator.dev.yugabyte.com/D15114
@arybochkin arybochkin moved this from In progress to Done in Tablet splitting Feb 22, 2022
@arybochkin arybochkin moved this from In progress to Done in YBase features Feb 22, 2022
jayant07-yb pushed a commit to jayant07-yb/yugabyte-db that referenced this issue Mar 8, 2022
@arybochkin
[yugabyte#10163] docdb: Regular DB special records could affect the c…
6ba29e3 
…orrectness of a middle key calculation for split

Summary:
The presence of special records inside regular RocksDB could affect the correctness of middle key
calculation. Having a special key as the split key will affect the correctness of tablet spitting.
Returning an error status is enough in that case as this will delay the tablet spitting (tablet
will receive more records and this will lead to a different middle key being picked). Most likely
this error will never be hit as the number of special records is negligibly small and they sort
before any user record. Special keys are `(kTransactionApplyState, 7)` for regular DB,
`(kExternalTransactionId, 8)`and `(kTransactionId, 'x')` for intents-db.

This fix updates `Tablet::GetEncodedMiddleSplitKey()` to throw an error when a special key is
selected as the middle key (and some other cases where a more sensible error is needed).
Corresponding tests are also added both for hash and range partitioning as DocKeys are generated in
a slightly different way depending on partitioning. Also several test routines were updated
to provide handling with respect to current partitioning.

Test Plan:
ybd --cxx-test integration-tests_tablet-split-itest --gtest_filter "*TabletSplitSystemRecordsITest.GetSplitKey*"
ybd --cxx-test integration-tests_tablet-split-itest
ybd --cxx-test tablet_tablet-split-test
ybd --cxx-test client_ql-tablet-test --gtest_filter QLTabletTest.GetMiddleKey

Reviewers: timur, rsami

Reviewed By: rsami

Subscribers: mbautin, ybase

Differential Revision: https://phabricator.dev.yugabyte.com/D15114
arybochkin added a commit that referenced this issue Mar 18, 2022
@arybochkin
[BACKPORT 2.12][#10163] docdb: Regular DB special records could affec…
06ef315 
…t the correctness of a middle key calculation for split

Summary:
Original commit: fb24aea / D15114

The presence of special records inside regular RocksDB could affect the correctness of middle key
calculation. Having a special key as the split key will affect the correctness of tablet spitting.
Returning an error status is enough in that case as this will delay the tablet spitting (tablet
will receive more records and this will lead to a different middle key being picked). Most likely
this error will never be hit as the number of special records is negligibly small and they sort
before any user record. Special keys are (kTransactionApplyState, 7) for regular DB,
(kExternalTransactionId, 8)and (kTransactionId, 'x') for intents-db.

This fix updates Tablet::GetEncodedMiddleSplitKey() to throw an error when a special key is
selected as the middle key (and some other cases where a more sensible error is needed).
Corresponding tests are also added both for hash and range partitioning as DocKeys are generated in
a slightly different way depending on partitioning. Also several test routines were updated
to provide handling with respect to current partitioning.

Test Plan:
ybd --cxx-test integration-tests_tablet-split-itest --gtest_filter "*TabletSplitSystemRecordsITest.GetSplitKey*"
ybd --cxx-test integration-tests_tablet-split-itest
ybd --cxx-test tablet_tablet-split-test
ybd --cxx-test client_ql-tablet-test --gtest_filter QLTabletTest.GetMiddleKey

Reviewers: rsami

Reviewed By: rsami

Subscribers: ybase

Differential Revision: https://phabricator.dev.yugabyte.com/D15830
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@arybochkin arybochkin

Labels
area/docdb YugabyteDB core features kind/bug This issue is a bug priority/high High Priority
Projects
Tablet splitting
  
Done
YBase features
  
Done
Milestone
No milestone
Development

No branches or pull requests
4 participants
@bmatican @ttyusupov @rthallamko3 @arybochkin