Skip to content
This repository has been archived by the owner on Nov 24, 2023. It is now read-only.

master: add etcd compaction and quota config #1521

Merged
merged 5 commits into from
Mar 25, 2021

Conversation

lance6716
Copy link
Collaborator

@lance6716 lance6716 commented Mar 19, 2021

What problem does this PR solve?

part of #1519

What is changed and how it works?

  • set default values and expose etcd config for auto-compaction and backend-quota
  • enable auto-compaction for a short value in integration test, to see if DM could handle compaction error

Check List

Tests

  • Unit test
  • Integration test to check DM could handle compaction error

Code changes

  • Has exported variable/fields change
  • Has persistent data change

Side effects

Related changes

  • Need to cherry-pick to the release branch
  • Need to update the documentation
  • Need to be included in the release note

@lance6716
Copy link
Collaborator Author

I tested setting and not setting auto comapction configurations, with writing different value to a same key. The DBSize of endpoint status has no difference most time, while for one configuration old revision key is compacted and other is not.

Will check DBSize of endpoint status in integration test later.

@lance6716
Copy link
Collaborator Author

lance6716 commented Mar 19, 2021

etcd-io/etcd#8496

according to above link, auto compaction does no help to disk problems, and defregment will stop etcd serving thus need carefully trigger, we might let user configurate a big quota to reduce the chance, and write an FAQ on etcd defregment or provide a helper command

--

update: we might implement rolling defregment in future

@lance6716
Copy link
Collaborator Author

/run-all-tests

@lance6716 lance6716 added needs-cherry-pick-release-2.0 This PR should be cherry-picked to release-2.0. Remove this label after cherry-picked to release-2.0 needs-update-release-note This PR should be added into release notes. Remove this label once the release notes are updated status/PTAL This PR is ready for review. Add this label back after committing new changes labels Mar 22, 2021
@lance6716 lance6716 changed the title [WIP] master: add etcd compaction and quota config master: add etcd compaction and quota config Mar 22, 2021
@lance6716 lance6716 added this to the v2.0.2 milestone Mar 22, 2021
@lichunzhu
Copy link
Contributor

/lgtm
But if we want to add this in v2.0.2, we'd better and periodically defragment too.

@ti-chi-bot ti-chi-bot added the status/LGT1 One reviewer already commented LGTM label Mar 24, 2021
@lance6716
Copy link
Collaborator Author

But if we want to add this in v2.0.2, we'd better and periodically defragment too.

I think adding this and lacking defragment didn't make things worse. defragment will stop etcd's serving, thus we should let master retire leader to run a defragment. I'm not clear about the logic now

@lichunzhu
Copy link
Contributor

Users can't achieve their goals if they don't do defragment manually. They may think auto-compaction can avoid Error: etcdserver: mvcc: database space exceeded problem. But actually, it's insufficient.

@lance6716
Copy link
Collaborator Author

Users can't achieve their goals if they don't do defragment manually. They may think auto-compaction can avoid Error: etcdserver: mvcc: database space exceeded problem. But actually, it's insufficient.

we could tell them auto-compaction features are the same behaviour with etcd/pd, so their expectation is wrong.

@lichunzhu
Copy link
Contributor

lichunzhu commented Mar 24, 2021

Users can't achieve their goals if they don't do defragment manually. They may think auto-compaction can avoid Error: etcdserver: mvcc: database space exceeded problem. But actually, it's insufficient.

we could tell them auto-compaction features are the same behaviour with etcd/pd, so their expectation is wrong.

OK. We should add this to our documents. I'm curious about how pd handles this problem? I mean how to tell users to use defragment. I check their code there is no periodically defragment, either.

Copy link
Collaborator

@GMHDBJD GMHDBJD left a comment

Choose a reason for hiding this comment

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

We may no need to config `auto-compaction-retention in so many integration tests. I think most users will not manually config it.
rest LGTM

@lance6716
Copy link
Collaborator Author

We may no need to config `auto-compaction-retention in so many integration tests. I think most users will not manually config it.
rest LGTM

now we set the efault value to 1h so DM may face etcd compacted error. I set it to a shorter value in integration test to expose failure of etcd compacted error

Copy link
Collaborator

@GMHDBJD GMHDBJD 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-chi-bot
Copy link
Member

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • GMHDBJD
  • lichunzhu

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 writing /lgtm in a comment.
Reviewer can cancel approval by writing /lgtm cancel in a comment.

@ti-chi-bot ti-chi-bot added status/LGT2 Two reviewers already commented LGTM, ready for merge and removed status/LGT1 One reviewer already commented LGTM labels Mar 25, 2021
@lance6716
Copy link
Collaborator Author

/merge

@ti-chi-bot
Copy link
Member

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

Commit hash: 40b63e8

@ti-chi-bot ti-chi-bot merged commit b40c0ea into pingcap:master Mar 25, 2021
ti-srebot pushed a commit to ti-srebot/dm that referenced this pull request Mar 25, 2021
@ti-srebot
Copy link

cherry pick to release-2.0 in PR #1534

@ti-srebot ti-srebot added already-cherry-pick-2.0 The related PR is already cherry-picked to release-2.0. Add this label once the PR is cherry-picked and removed needs-cherry-pick-release-2.0 This PR should be cherry-picked to release-2.0. Remove this label after cherry-picked to release-2.0 labels Mar 25, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
already-cherry-pick-2.0 The related PR is already cherry-picked to release-2.0. Add this label once the PR is cherry-picked needs-update-release-note This PR should be added into release notes. Remove this label once the release notes are updated size/L status/can-merge status/LGT2 Two reviewers already commented LGTM, ready for merge status/PTAL This PR is ready for review. Add this label back after committing new changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants