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

store/tikv: resolve storeType dependency #22854

Merged
merged 7 commits into from
Feb 22, 2021
Merged

store/tikv: resolve storeType dependency #22854

merged 7 commits into from
Feb 22, 2021

Conversation

disksing
Copy link
Contributor

Signed-off-by: disksing [email protected]

What problem does this PR solve?

store/tikv has dependencies on kv and placement to distinguish store types.
Part of #22513

What is changed and how it works?

What's Changed:

  • copy store type definations to store/tikv
  • rename TiKvOnly to TiKVOnly

Check List

Tests

  • Unit test

Release note

  • No release note

@disksing disksing added the sig/transaction SIG:Transaction label Feb 20, 2021
AndreMouche
AndreMouche previously approved these changes Feb 22, 2021
Copy link
Contributor

@AndreMouche AndreMouche 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 Indicates that a PR has LGTM 1. label Feb 22, 2021
@disksing
Copy link
Contributor Author

/run-all-tests

kv/kv.go Outdated
@@ -343,6 +343,7 @@ const (
)

// StoreType represents the type of a store.
// Type enums should sync with store/tikv.StoreType.
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not unify these two StoreType?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

good point.

Copy link
Contributor Author

@disksing disksing Feb 22, 2021

Choose a reason for hiding this comment

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

@andylokandy I discover that the 2 storeTypes are actually not strict related. There is no need to keep them synced. (I even tried to change tikv.StoreType to type string, it can still compile). So I removed the comments that declare they should be sync.

Signed-off-by: disksing <[email protected]>
@andylokandy
Copy link
Contributor

LGTM

@ti-srebot ti-srebot removed the status/LGT1 Indicates that a PR has LGTM 1. label Feb 22, 2021
@ti-srebot ti-srebot added the status/LGT2 Indicates that a PR has LGTM 2. label Feb 22, 2021
@disksing
Copy link
Contributor Author

/merge

@ti-srebot ti-srebot added the status/can-merge Indicates a PR has been approved by a committer. label Feb 22, 2021
@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@disksing merge failed.

@disksing
Copy link
Contributor Author

/merge

@ti-srebot
Copy link
Contributor

Your auto merge job has been accepted, waiting for:

  • 22855
  • 22866

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot
Copy link
Contributor

@disksing merge failed.

@disksing
Copy link
Contributor Author

/merge

@ti-srebot
Copy link
Contributor

Your auto merge job has been accepted, waiting for:

  • 22418
  • 22868

@ti-srebot
Copy link
Contributor

/run-all-tests

@ti-srebot ti-srebot merged commit 90ee6c6 into pingcap:master Feb 22, 2021
@disksing disksing deleted the storetype branch February 22, 2021 11:01
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 Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants