Skip to content
This repository was archived by the owner on Aug 21, 2023. It is now read-only.

Conversation

docsir
Copy link
Contributor

@docsir docsir commented Oct 18, 2021

What problem does this PR solve?

Close #370

What is changed and how it works?

Add error message when --consistency is lock and --snapshot parameter is provided.

Check List

Tests

  • Unit test
  • Manual test (add detailed scripts or steps below)

Release note

No release note

@ti-chi-bot
Copy link
Member

[REVIEW NOTIFICATION]

This pull request has not been approved.

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.

}

func validateResolveAutoConsistency(conf *Config) error {
if conf.Consistency == consistencyTypeSnapshot || conf.Snapshot != "" {
Copy link
Contributor

Choose a reason for hiding this comment

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

function validateResolveAutoConsistency should be checked after resolveAutoConsistency.

Suggested change
if conf.Consistency == consistencyTypeSnapshot || conf.Snapshot != "" {
if conf.Consistency != consistencyTypeSnapshot && conf.Snapshot != "" {


func validateResolveAutoConsistency(conf *Config) error {
if conf.Consistency == consistencyTypeSnapshot || conf.Snapshot != "" {
return errors.New("can't specify both --consistency and --snapshot at the same time. snapshot consistency is not supported for this server")
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
return errors.New("can't specify both --consistency and --snapshot at the same time. snapshot consistency is not supported for this server")
return errors.Errorf("can't specify --snapshot when --consistency isn't snapshot, resolved consistency: %s", conf.Consistency)

err := adjustConfig(conf,
registerTLSConfig,
validateSpecifiedSQL,
validateResolveAutoConsistency,
Copy link
Contributor

Choose a reason for hiding this comment

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

Function validateResolveAutoConsistency should be after resolveAutoConsistency.

Copy link
Contributor

@lichunzhu lichunzhu left a comment

Choose a reason for hiding this comment

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

Rest LGTM

testCases := []struct {
confConsistency string
confSnapshot string
err error
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems that using string here is enough.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Dumpling should report error if --consistency is lock and --snapshot parameter is provided

4 participants