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

*: rollback schema in the tracker; fix save table checkpoint in optimistic mode #625

Merged
merged 27 commits into from
Apr 28, 2020

Conversation

csuzhangxc
Copy link
Member

@csuzhangxc csuzhangxc commented Apr 21, 2020

What problem does this PR solve?

fix #585.

What is changed and how it works?

rollback tracked schema when pausing/stopping.

Check List

Tests

  • Unit test
  • Integration test

Code changes

  • Has persistent data change

Side effects

  • Increased code complexity

@csuzhangxc csuzhangxc added the status/WIP This PR is still work in progress label Apr 21, 2020
@csuzhangxc
Copy link
Member Author

/run-all-tests

@@ -438,6 +438,13 @@ func (p *Pessimist) handleInfoPut(ctx context.Context, infoCh <-chan pessimism.I
lockID, synced, remain, err := p.lk.TrySync(info, p.taskSources(info.Task))
if err != nil {
// TODO: add & update metrics.
// FIXME: the following case is not supported automatically now, try to support it later.
Copy link
Member Author

Choose a reason for hiding this comment

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

"\"result\": true" 3

# NOTE: the lock may be locked for the next DDL, for details please see the following comments in `master/shardll/pessimist.go`,
# `FIXME: the following case is not supported automatically now, try to support it later`
Copy link
Member Author

Choose a reason for hiding this comment

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

if point.rollback() {
logger.Debug("try to rollback checkpoint", log.WrapStringerField("checkpoint", point))
from := point.MySQLLocation()
if point.rollback(schemaTracker, schema) {
Copy link
Member Author

Choose a reason for hiding this comment

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

also see #629.

@csuzhangxc csuzhangxc added priority/normal Minor change, requires approval from ≥1 primary reviewer type/bug-fix Bug fix labels Apr 23, 2020
b.Lock()
defer b.Unlock()
b.location = b.flushedLocation
if isSchemaChanged = b.ti != b.flushedTI; isSchemaChanged {
b.location = b.flushedLocation.Clone()
Copy link
Member Author

Choose a reason for hiding this comment

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

also .Clone.

@csuzhangxc
Copy link
Member Author

/run-all-tests

@codecov
Copy link

codecov bot commented Apr 23, 2020

Codecov Report

Merging #625 into master will increase coverage by 0.0266%.
The diff coverage is 14.5161%.

@@               Coverage Diff                @@
##             master       #625        +/-   ##
================================================
+ Coverage   57.7114%   57.7381%   +0.0266%     
================================================
  Files           202        202                
  Lines         20502      20496         -6     
================================================
+ Hits          11832      11834         +2     
+ Misses         7523       7516         -7     
+ Partials       1147       1146         -1     

@csuzhangxc csuzhangxc requested a review from lichunzhu April 23, 2020 07:48
@csuzhangxc
Copy link
Member Author

@WangXiangUSTC @lichunzhu PTAL

@lichunzhu lichunzhu added status/PTAL This PR is ready for review. Add this label back after committing new changes and removed status/WIP This PR is still work in progress labels Apr 23, 2020
@csuzhangxc csuzhangxc added this to the v2.0.0 beta.1 milestone Apr 24, 2020
s.tctx.L().Info("save table checkpoint", zap.String("event", "query"),
zap.String("schema", upSchema), zap.String("table", upTable),
zap.Strings("ddls", needHandleDDLs), log.WrapStringerField("location", ec.currentLocation))
s.saveTablePoint(upSchema, upTable, ec.currentLocation.Clone())
Copy link
Member Author

Choose a reason for hiding this comment

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

found a bug relative to #584.

@csuzhangxc csuzhangxc changed the title *: rollback schema in the tracker *: rollback schema in the tracker; fix save table checkpoint in optimistic mode Apr 24, 2020

log.L().Info("set FlushCheckpointStage", zap.String("failpoint", "FlushCheckpointStage"), zap.Int("stage", testInjector.flushCheckpointStage))
if testInjector.flushCheckpointStage == maxStage {
testInjector.flushCheckpointStage = -1 // disable for following stages.
Copy link
Contributor

Choose a reason for hiding this comment

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

After set flushCheckpointStage to -1, will flushCheckpointStage increase to maxStage later?

Copy link
Member Author

Choose a reason for hiding this comment

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

No, So how about setting it to MaxInt?

Copy link
Contributor

Choose a reason for hiding this comment

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

I understand it. I think -1 is just OK.

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

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.

LGTM

@lichunzhu lichunzhu added status/LGT1 One reviewer already commented LGTM and removed status/PTAL This PR is ready for review. Add this label back after committing new changes labels Apr 27, 2020
Copy link
Contributor

@WangXiangUSTC WangXiangUSTC 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

// if any of them are not equal, then we rollback them:
// - set the one in the checkpoint but not flushed to the one flushed.
// - set the one tracked to the one in the checkpoint by the caller of this method (both flushed and not flushed are the same now)
if isSchemaChanged = trackedTi != b.ti || b.ti != b.flushedTI; isSchemaChanged {
Copy link
Contributor

Choose a reason for hiding this comment

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

how about adding some ()

Copy link
Member Author

Choose a reason for hiding this comment

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

added in 846c6b5.

Copy link
Contributor

@WangXiangUSTC WangXiangUSTC left a comment

Choose a reason for hiding this comment

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

LGTM

@csuzhangxc csuzhangxc merged commit 49300b4 into pingcap:master Apr 28, 2020
@csuzhangxc csuzhangxc deleted the revert-tracker branch April 28, 2020 07:35
Kuri-su pushed a commit to Kur-Public/dm that referenced this pull request Apr 28, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
priority/normal Minor change, requires approval from ≥1 primary reviewer status/LGT1 One reviewer already commented LGTM type/bug-fix Bug fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

reset schema tracker after resume
3 participants