engine_trait: introduce status error #13059

BusyJay · 2022-07-19T12:27:46Z

What is changed and how it works?

Issue Number: Ref #13058

What's Changed:

This PR is the first step to prepare for tirocks, the new rocksdb wrapper.

The status error is introduced and iterator trait is refactored to keep
consistent with tirocks.

Check List

Tests

Unit test
Integration test

Release note

None

This PR is the first step to prepare for tirocks, the new rocksdb wrapper. The status error is introduced and iterator trait is refactored to keep consistent with tirocks. Signed-off-by: Jay Lee <BusyJayLee@gmail.com>

ti-chi-bot · 2022-07-19T12:27:47Z

[REVIEW NOTIFICATION]

This pull request has been approved by:

Connor1996
tabokie

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

components/engine_traits/src/iterable.rs

Co-authored-by: Xinye Tao <xy.tao@outlook.com> Signed-off-by: Jay Lee <BusyJayLee@gmail.com>

Connor1996 · 2022-07-20T06:40:19Z

components/engine_rocks/src/status.rs

+/// A macro that will transform a rocksdb error to engine trait error.
+///
+/// r stands for rocksdb, e stands for engine_trait.
+macro_rules! r2e {


can we implement From for error and avoid using macro

No, because neither types are defined by engine_rocks.

how about adding intermediate type in engine_rocks

Then the type needs to be constructed every where. You can't build up such connections that just using ? to convert two foreign types.

how about shortening from_rocksdb_error to r2e. Writing xxx().map_err(r2e) is more readible than r2e!(xxx())

tonyxuqqi · 2022-07-20T22:16:36Z

components/engine_rocks/src/status.rs

+/// A macro that will transform a rocksdb error to engine trait error.
+///
+/// r stands for rocksdb, e stands for engine_trait.
+macro_rules! r2e {


what about rocks2engine_error so it's no need to see the definition to understand what it does.

The name is too long to be written everywhere.

tonyxuqqi · 2022-07-20T22:17:11Z

components/engine_rocks/src/status.rs

+/// A trait that will transform a engine trait error to rocksdb error.
+///
+/// r stands for rocksdb, e stands for engine_trait.
+macro_rules! e2r {


same here, engine2rocks_error?

Signed-off-by: Jay Lee <BusyJayLee@gmail.com>

Connor1996

LGTM

BusyJay · 2022-07-22T02:37:27Z

/merge

ti-chi-bot · 2022-07-22T02:37:28Z

@BusyJay: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and if the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

If you have any questions about the PR merge process, please refer to pr process.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

ti-chi-bot · 2022-07-22T02:37:30Z

This pull request has been accepted and is ready to merge.

Commit hash: 6a07901

BusyJay · 2022-07-22T03:48:16Z

/test

Signed-off-by: Jay Lee <BusyJayLee@gmail.com>

…duce-status Signed-off-by: Jay Lee <BusyJayLee@gmail.com>

Signed-off-by: Jay Lee <BusyJayLee@gmail.com>

BusyJay · 2022-07-22T07:29:30Z

/merge

ti-chi-bot · 2022-07-22T07:29:31Z

@BusyJay: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and if the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

If you have any questions about the PR merge process, please refer to pr process.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

ti-chi-bot · 2022-07-22T07:29:34Z

This pull request has been accepted and is ready to merge.

Commit hash: 3ece129

ti-chi-bot · 2022-07-22T07:29:47Z

@BusyJay: Your PR was out of date, I have automatically updated it for you.

At the same time I will also trigger all tests for you:

/run-all-tests

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

Signed-off-by: Jay Lee <BusyJayLee@gmail.com>

…duce-status Signed-off-by: Jay Lee <BusyJayLee@gmail.com>

BusyJay · 2022-07-22T08:41:20Z

/merge

ti-chi-bot · 2022-07-22T08:41:20Z

@BusyJay: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

You only need to trigger /merge once, and if the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

If you have any questions about the PR merge process, please refer to pr process.

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

ti-chi-bot · 2022-07-22T08:41:23Z

This pull request has been accepted and is ready to merge.

Commit hash: 294a558

ref tikv#13058 This PR is the first step to prepare for tirocks, the new rocksdb wrapper. The status error is introduced and iterator trait is refactored to keep consistent with tirocks. Signed-off-by: Jay Lee <BusyJayLee@gmail.com> Co-authored-by: Ti Chi Robot <ti-community-prow-bot@tidb.io> Co-authored-by: Xinye Tao <xy.tao@outlook.com>

engine_trait: introduce status error

974f453

This PR is the first step to prepare for tirocks, the new rocksdb wrapper. The status error is introduced and iterator trait is refactored to keep consistent with tirocks. Signed-off-by: Jay Lee <BusyJayLee@gmail.com>

BusyJay requested review from Connor1996 and tabokie July 19, 2022 12:27

ti-chi-bot added release-note-none Denotes a PR that doesn't merit a release note. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels Jul 19, 2022

tabokie approved these changes Jul 20, 2022

View reviewed changes

components/engine_traits/src/iterable.rs Outdated Show resolved Hide resolved

components/engine_traits/src/iterable.rs Outdated Show resolved Hide resolved

ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Jul 20, 2022

Apply suggestions from code review

3bea3c9

Co-authored-by: Xinye Tao <xy.tao@outlook.com> Signed-off-by: Jay Lee <BusyJayLee@gmail.com>

BusyJay force-pushed the introduce-status branch from 9c121a1 to 3bea3c9 Compare July 20, 2022 05:10

Connor1996 reviewed Jul 20, 2022

View reviewed changes

ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 20, 2022

tonyxuqqi reviewed Jul 20, 2022

View reviewed changes

BusyJay added 3 commits July 21, 2022 18:04

address comment

18fa280

Signed-off-by: Jay Lee <BusyJayLee@gmail.com>

Merge branch 'master' into introduce-status

cf9d7d5

Signed-off-by: Jay Lee <BusyJayLee@gmail.com>

fix compile error

6a07901

Signed-off-by: Jay Lee <BusyJayLee@gmail.com>

Connor1996 approved these changes Jul 21, 2022

View reviewed changes

ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. labels Jul 21, 2022

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Jul 22, 2022

Merge branch 'master' into introduce-status

90e1916

BusyJay added 2 commits July 22, 2022 12:06

unwrap error

a9b0ded

Signed-off-by: Jay Lee <BusyJayLee@gmail.com>

Merge branch 'introduce-status' of github.com:busyjay/tikv into intro…

c5aac7a

…duce-status Signed-off-by: Jay Lee <BusyJayLee@gmail.com>

ti-chi-bot removed the status/can-merge Indicates a PR has been approved by a committer. label Jul 22, 2022

BusyJay added 2 commits July 22, 2022 15:06

skip invalid environment check

ae29d16

Signed-off-by: Jay Lee <BusyJayLee@gmail.com>

fix clippy

3ece129

Signed-off-by: Jay Lee <BusyJayLee@gmail.com>

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Jul 22, 2022

Merge branch 'master' into introduce-status

a506cd1

BusyJay added 2 commits July 22, 2022 16:39

make test_stale_read_while_applying_snapshot stable

6cc7430

Signed-off-by: Jay Lee <BusyJayLee@gmail.com>

Merge branch 'introduce-status' of github.com:busyjay/tikv into intro…

294a558

…duce-status Signed-off-by: Jay Lee <BusyJayLee@gmail.com>

ti-chi-bot removed the status/can-merge Indicates a PR has been approved by a committer. label Jul 22, 2022

ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Jul 22, 2022

ti-chi-bot merged commit 6e66f09 into tikv:master Jul 22, 2022

ti-chi-bot added this to the Pool milestone Jul 22, 2022

BusyJay deleted the introduce-status branch July 23, 2022 03:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

engine_trait: introduce status error #13059

engine_trait: introduce status error #13059

BusyJay commented Jul 19, 2022

ti-chi-bot commented Jul 19, 2022 •

edited

Loading

Connor1996 Jul 20, 2022

BusyJay Jul 20, 2022

Connor1996 Jul 21, 2022

BusyJay Jul 21, 2022

Connor1996 Jul 21, 2022

tonyxuqqi Jul 20, 2022

BusyJay Jul 21, 2022

tonyxuqqi Jul 20, 2022

Connor1996 left a comment

BusyJay commented Jul 22, 2022

ti-chi-bot commented Jul 22, 2022

ti-chi-bot commented Jul 22, 2022

BusyJay commented Jul 22, 2022

BusyJay commented Jul 22, 2022

ti-chi-bot commented Jul 22, 2022

ti-chi-bot commented Jul 22, 2022

ti-chi-bot commented Jul 22, 2022

BusyJay commented Jul 22, 2022

ti-chi-bot commented Jul 22, 2022

ti-chi-bot commented Jul 22, 2022

engine_trait: introduce status error #13059

engine_trait: introduce status error #13059

Conversation

BusyJay commented Jul 19, 2022

What is changed and how it works?

Check List

Release note

ti-chi-bot commented Jul 19, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Connor1996 left a comment

Choose a reason for hiding this comment

BusyJay commented Jul 22, 2022

ti-chi-bot commented Jul 22, 2022

ti-chi-bot commented Jul 22, 2022

BusyJay commented Jul 22, 2022

BusyJay commented Jul 22, 2022

ti-chi-bot commented Jul 22, 2022

ti-chi-bot commented Jul 22, 2022

ti-chi-bot commented Jul 22, 2022

BusyJay commented Jul 22, 2022

ti-chi-bot commented Jul 22, 2022

ti-chi-bot commented Jul 22, 2022

ti-chi-bot commented Jul 19, 2022 •

edited

Loading