Skip to content

store/tikv: tiny refactoring, change twoPhaseCommitAction to interface#12845

Merged
sre-bot merged 2 commits intopingcap:masterfrom
tiancaiamao:two-phase-action
Oct 21, 2019
Merged

store/tikv: tiny refactoring, change twoPhaseCommitAction to interface#12845
sre-bot merged 2 commits intopingcap:masterfrom
tiancaiamao:two-phase-action

Conversation

@tiancaiamao
Copy link
Contributor

@tiancaiamao tiancaiamao commented Oct 21, 2019

What problem does this PR solve?

We define type twoPhaseCommitAction int and use switch case on them.

func (ca twoPhaseCommitAction) String() string {
	switch ca {
	case actionPrewrite:
		return "prewrite"
	case actionCommit:
		return "commit"
	case actionCleanup:
		return "cleanup"
	case actionPessimisticLock:
		return "pessimistic_lock"
	case actionPessimisticRollback:
		return "pessimistic_rollback"
	}
	return "unknown"
}
func (c *twoPhaseCommitter) getProcFuncByType(action twoPhaseCommitAction) (procOneBatchFn, error) {
	var singleBatchActionFunc procOneBatchFn
	switch action {
	case actionPrewrite:
		singleBatchActionFunc = c.prewriteSingleBatch
	case actionCommit:
		singleBatchActionFunc = c.commitSingleBatch
	case actionCleanup:
		singleBatchActionFunc = c.cleanupSingleBatch
	case actionPessimisticLock:
		singleBatchActionFunc = c.pessimisticLockSingleBatch
	case actionPessimisticRollback:
		singleBatchActionFunc = c.pessimisticRollbackSingleBatch
	default:
		return nil, errors.Errorf("invalid action type=%v", action)
	}
	return singleBatchActionFunc, nil
}

I need to closure some data on pessimisticLockSingleBatch, but I think it's improper to change the

type procOneBatchFn func(bo *Backoffer, batch batchKeys) error

to something like

func(bo *Backoffer, killed *uint32, batch batchKeys) 

so I think using interface is more flexible.

What is changed and how it works?

change twoPhaseCommitAction to interface.

Check List

Tests

  • No code

Related changes

  • Need to cherry-pick to the release branch
    This refactoring is prepared for an incoming bug-fix PR

@tiancaiamao tiancaiamao added type/enhancement The issue or PR belongs to an enhancement. needs-cherry-pick-3.0 labels Oct 21, 2019
@tiancaiamao
Copy link
Contributor Author

PTAL @coocood @lysu @cfzjywxk

Copy link
Contributor

@lysu lysu left a comment

Choose a reason for hiding this comment

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

LGTM

type actionCommit struct{}
type actionCleanup struct{}
type actionPessimisticLock struct{}
type actionPessimisticRollback struct{}
Copy link
Contributor

Choose a reason for hiding this comment

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

type (
   actionPrewrite struct{}
   actionCommit struct{}
   actionCleanup struct{}
   actionPessimisticLock struct{}
   actionPessimisticRollback struct{}
)

we can avoid duplicate type words~

@lysu lysu added the status/LGT1 Indicates that a PR has LGTM 1. label Oct 21, 2019
Copy link
Contributor

@jackysp jackysp left a comment

Choose a reason for hiding this comment

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

LGTM

@cfzjywxk
Copy link
Contributor

LGTM

Copy link
Member

@coocood coocood left a comment

Choose a reason for hiding this comment

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

LGTM

@coocood
Copy link
Member

coocood commented Oct 21, 2019

/run-unit-test

@jackysp jackysp added the status/can-merge Indicates a PR has been approved by a committer. label Oct 21, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Oct 21, 2019

Your auto merge job has been accepted, waiting for 12608

@codecov
Copy link

codecov bot commented Oct 21, 2019

Codecov Report

Merging #12845 into master will not change coverage.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             master     #12845   +/-   ##
===========================================
  Coverage   80.0972%   80.0972%           
===========================================
  Files           465        465           
  Lines        106980     106980           
===========================================
  Hits          85688      85688           
  Misses        14893      14893           
  Partials       6399       6399

@sre-bot
Copy link
Contributor

sre-bot commented Oct 21, 2019

/run-all-tests

@sre-bot sre-bot merged commit 16f3e6a into pingcap:master Oct 21, 2019
@sre-bot
Copy link
Contributor

sre-bot commented Oct 21, 2019

cherry pick to release-3.1 failed

@sre-bot
Copy link
Contributor

sre-bot commented Oct 21, 2019

cherry pick to release-3.0 failed

@crazycs520
Copy link
Contributor

@tiancaiamao Need cherry pick to 3.1?

XiaTianliang pushed a commit to XiaTianliang/tidb that referenced this pull request Dec 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status/can-merge Indicates a PR has been approved by a committer. status/LGT1 Indicates that a PR has LGTM 1. type/enhancement The issue or PR belongs to an enhancement.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants