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

*: show an unsynced message for workers which didn't get any shard DDL #1638

Merged
merged 8 commits into from
May 6, 2021

Conversation

lance6716
Copy link
Collaborator

@lance6716 lance6716 commented Apr 27, 2021

What problem does this PR solve?

some users don't use show-ddl-locks to get unsynced tables, they use query-status instead, so we also show reasonable message in query-status

What is changed and how it works?

Check List

Tests

  • Integration test

Code changes

  • Has (user) interface methods change

Related changes

  • Need to cherry-pick to the release branch
  • Need to be included in the release note

@lance6716 lance6716 added 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 and removed size/M labels Apr 27, 2021
@lance6716 lance6716 added this to the v2.0.3 milestone Apr 27, 2021
@ti-chi-bot
Copy link
Member

@Ehco1996: /lgtm is only allowed for the reviewers in list.

In response to this:

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the ti-community-infra/tichi repository.

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

@ti-chi-bot ti-chi-bot added the status/LGT1 One reviewer already commented LGTM label Apr 30, 2021
Comment on lines 696 to 731
unsyncedMap := make(map[string]map[string]struct{})
for _, resp := range resps {
for _, subtaskStatus := range resp.SubTaskStatus {
syncStatus := subtaskStatus.GetSync()
if syncStatus == nil {
continue
}
for _, group := range syncStatus.UnresolvedGroups {
if _, ok := unsyncedMap[subtaskStatus.Name]; !ok {
unsyncedMap[subtaskStatus.Name] = make(map[string]struct{})
}
unsyncedMap[subtaskStatus.Name][group.Target] = struct{}{}
}
}
}
for _, resp := range resps {
for _, subtaskStatus := range resp.SubTaskStatus {
syncStatus := subtaskStatus.GetSync()
if syncStatus == nil {
continue
}
found := make(map[string]struct{}, len(syncStatus.UnresolvedGroups))
for _, group := range syncStatus.UnresolvedGroups {
found[group.Target] = struct{}{}
}
for target := range unsyncedMap[subtaskStatus.Name] {
if _, ok := found[target]; ok {
continue
}
syncStatus.UnresolvedGroups = append(syncStatus.UnresolvedGroups, &pb.ShardingGroup{
Target: target,
Unsynced: []string{"this DM-worker doesn't receive any shard DDL of this group"},
})
}
}
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Weak advice: can we use a separate function for this part and add a unit test for it?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@lance6716
Copy link
Collaborator Author

/hold

@ti-chi-bot ti-chi-bot added size/L and removed size/M labels Apr 30, 2021
@lance6716
Copy link
Collaborator Author

/unhold

@lance6716
Copy link
Collaborator Author

/hold

If user only queries one source not whole task, current code is not awared of unsynced workers. So I will change the implementation to use ShowLocks of pessimist/optimist

@lance6716
Copy link
Collaborator Author

/unhold

@GMHDBJD
Copy link
Collaborator

GMHDBJD commented May 6, 2021

any test for optimistic mode?

@lance6716
Copy link
Collaborator Author

any test for optimistic mode?

seems optimistic mode will discover sources in a shard group lazily, so a totally unsynchronized DM-worker is not counted into shard group. will check later

@lance6716
Copy link
Collaborator Author

Oh currently query-status doesn't show unresolvedGroups for optimistic tasks. I'll add a TODO

// only support to show `UnresolvedGroups` in pessimistic mode now.

Copy link
Collaborator

@GMHDBJD GMHDBJD 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-chi-bot
Copy link
Member

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • GMHDBJD
  • lichunzhu

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 writing /lgtm in a comment.
Reviewer can cancel approval by writing /lgtm cancel in a comment.

@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 May 6, 2021
@lance6716
Copy link
Collaborator Author

/merge

@ti-chi-bot
Copy link
Member

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

Commit hash: 8f56e5e

@ti-chi-bot ti-chi-bot merged commit c9ed290 into pingcap:master May 6, 2021
@ti-chi-bot
Copy link
Member

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

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants