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

optimistic: fix optimist panic after table/source removed #1971

Merged
merged 3 commits into from
Aug 11, 2021

Conversation

GMHDBJD
Copy link
Collaborator

@GMHDBJD GMHDBJD commented Aug 9, 2021

What problem does this PR solve?

close #1970

What is changed and how it works?

Check List

Tests

  • Unit test
  • Integration test

@ti-chi-bot
Copy link
Member

ti-chi-bot commented Aug 9, 2021

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • Ehco1996
  • lance6716

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.

@GMHDBJD GMHDBJD added status/WIP This PR is still work in progress type/bug-fix Bug fix needs-update-release-note This PR should be added into release notes. Remove this label once the release notes are updated 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 Aug 9, 2021
@GMHDBJD GMHDBJD added this to the v2.0.6 milestone Aug 9, 2021
@lance6716 lance6716 changed the title optimistic: fix master panic after optimistic: fix master panic after optimistic table/source removed Aug 9, 2021
@lance6716 lance6716 changed the title optimistic: fix master panic after optimistic table/source removed optimistic: fix optimist panic after table/source removed Aug 9, 2021
@GMHDBJD
Copy link
Collaborator Author

GMHDBJD commented Aug 10, 2021

/run-all-tests
execute statement failed: ALTER TABLE `shardddl`.`tb` ADD COLUMN `new_col1` INT, RawCause: Error 8028: Information schema is changed during the execution of the statement(for example, table definition may be updated by other DDL ran in parallel)

@GMHDBJD
Copy link
Collaborator Author

GMHDBJD commented Aug 10, 2021

/run-all-tests

@GMHDBJD GMHDBJD 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 Aug 10, 2021
}
// filter info which doesn't have SourceTable
// SourceTable will be changed after user update block-allow-list
Copy link
Contributor

@Ehco1996 Ehco1996 Aug 11, 2021

Choose a reason for hiding this comment

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

as opposed to cleaning up error etcd data here
maybe we can celar outated info in etcd after user update block-allow-list is a better idea?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Not reliable in current design.

user do a ddl, 
stop-task
drop table
redo task(start-task remove meta)

the ddl info still in etcd, but sourceTable was deleted.

Copy link
Contributor

Choose a reason for hiding this comment

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

can we clear data when remove-meta?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's a idea

@ti-chi-bot ti-chi-bot added the status/LGT1 One reviewer already commented LGTM label Aug 11, 2021
@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 Aug 11, 2021
@GMHDBJD
Copy link
Collaborator Author

GMHDBJD commented Aug 11, 2021

/merge

@ti-chi-bot
Copy link
Member

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

Commit hash: eafbfcb

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

@GMHDBJD
Copy link
Collaborator Author

GMHDBJD commented Aug 11, 2021

/run-all-tests

@ti-chi-bot
Copy link
Member

In response to a cherrypick label: new pull request created: #1984.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
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 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 type/bug-fix Bug fix
Projects
None yet
Development

Successfully merging this pull request may close these issues.

restart master will panic after some table remove from optimistic task
5 participants