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

storage: return memory locks as BatchGet response level error #9077

Merged
merged 11 commits into from Nov 24, 2020

Conversation

sticnarf
Copy link
Contributor

@sticnarf sticnarf commented Nov 20, 2020

What problem does this PR solve?

Issue Number: #9069

What is changed and how it works?

Set memory lock to the newly added KeyError field in BatchGetResponse. The client will skip using pairs if the error is used.

Related changes

Check List

Tests

  • Integration test

Release note

  • Part of the async commit feature

Signed-off-by: Yilin Chen <sticnarf@gmail.com>
Signed-off-by: Yilin Chen <sticnarf@gmail.com>
@sticnarf sticnarf marked this pull request as ready for review November 20, 2020 08:21
@tikv tikv deleted a comment from sre-bot Nov 20, 2020
Signed-off-by: Yilin Chen <sticnarf@gmail.com>
resp.set_pairs(pairs.into());
}
Err(e) => {
resp.set_error(extract_key_error(&e));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it backward compatible? What if an error except KeyIsLocked in replica read is reported?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed. It looks possible for async_snapshot to return some weird errors like Timeout or some unexpected errors. Then, an old client may not handle the error correctly.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I keep the error in the first kvpair. PTAL again.

Signed-off-by: Yilin Chen <sticnarf@gmail.com>
Copy link
Contributor

@youjiali1995 youjiali1995 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@ti-srebot ti-srebot added the status/LGT1 Status: PR - There is already 1 approval label Nov 23, 2020
@sticnarf
Copy link
Contributor Author

@MyonKeminta PTAL

Signed-off-by: Yilin Chen <sticnarf@gmail.com>
Copy link
Contributor

@MyonKeminta MyonKeminta left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ti-srebot ti-srebot removed the status/LGT1 Status: PR - There is already 1 approval label Nov 24, 2020
@ti-srebot ti-srebot added the status/LGT2 Status: PR - There are already 2 approvals label Nov 24, 2020
@sticnarf
Copy link
Contributor Author

/merge

@ti-srebot ti-srebot added the status/can-merge Status: Can merge to base branch label Nov 24, 2020
@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@sticnarf merge failed.

@sticnarf
Copy link
Contributor Author

/merge

@ti-srebot
Copy link
Contributor

Your auto merge job has been accepted, waiting for:

  • 9073

@ti-srebot
Copy link
Contributor

/run-all-tests

@sticnarf
Copy link
Contributor Author

/merge

@ti-srebot
Copy link
Contributor

Your auto merge job has been accepted, waiting for:

  • 9094

@ti-srebot
Copy link
Contributor

@sticnarf merge failed.

@ti-srebot
Copy link
Contributor

/run-all-tests

@sticnarf
Copy link
Contributor Author

/merge

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@sticnarf merge failed.

@sticnarf
Copy link
Contributor Author

/merge

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot ti-srebot merged commit 5ae480d into tikv:master Nov 24, 2020
longfangsong pushed a commit to longfangsong/tikv that referenced this pull request Nov 25, 2020
)

Signed-off-by: Yilin Chen <sticnarf@gmail.com>
@sticnarf sticnarf added this to In progress in Async Commit via automation Dec 29, 2020
@sticnarf sticnarf moved this from In progress to Done in Async Commit Dec 29, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
sig/transaction SIG: Transaction status/can-merge Status: Can merge to base branch status/LGT2 Status: PR - There are already 2 approvals type/bugfix Type: PR - Fix a bug
Projects
Async Commit
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants