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

check: add error/warn count for check-task #1610

Merged
merged 11 commits into from
Apr 22, 2021
Merged

check: add error/warn count for check-task #1610

merged 11 commits into from
Apr 22, 2021

Conversation

GMHDBJD
Copy link
Collaborator

@GMHDBJD GMHDBJD commented Apr 20, 2021

What problem does this PR solve?

close #1478

What is changed and how it works?

add error/warn count for check-task, default is 10

Check List

Tests

  • Integration test

@ti-chi-bot ti-chi-bot requested a review from lichunzhu April 20, 2021 07:04
@GMHDBJD GMHDBJD added status/WIP This PR is still work in progress type/bug-fix Bug fix 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 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 20, 2021
@lance6716 lance6716 added this to the v2.0.3 milestone Apr 20, 2021
@ti-chi-bot ti-chi-bot added the status/LGT1 One reviewer already commented LGTM label Apr 20, 2021
lichunzhu
lichunzhu reviewed Apr 21, 2021
View reviewed changes
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

dm/ctl/master/check_task.go Outdated
@@ -28,10 +28,12 @@ import (
// NewCheckTaskCmd creates a CheckTask command.
func NewCheckTaskCmd() *cobra.Command {
cmd := &cobra.Command{
Use: "check-task <config-file>",
Use: "check-task <config-file> [--errors] [--warns]",
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 not consistent with L52?
Actually we read error instead of errors.

lichunzhu
lichunzhu reviewed Apr 21, 2021
View reviewed changes
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

tests/dmctl_basic/check_list/check_task.sh

# 100 errors
run_dm_ctl $WORK_DIR "127.0.0.1:$MASTER_PORT" \
"check-task $cur/conf/dm-task3.yaml -e 100 -w 1" \
Copy link
Contributor

Choose a reason for hiding this comment

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

Use --error and --warn here?

Suggested change
"check-task $cur/conf/dm-task3.yaml -e 100 -w 1" \
"check-task $cur/conf/dm-task3.yaml --error 100 --warn 1" \

@lichunzhu
Copy link
Contributor

/lgtm

@ti-chi-bot
Copy link
Member

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • lance6716
  • 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 Apr 21, 2021
@lance6716
Copy link
Collaborator

/merge

@ti-chi-bot
Copy link
Member

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

Commit hash: 2a7c5e6

glorv
glorv reviewed Apr 21, 2021
View reviewed changes
checker/checker.go Outdated Show resolved Hide resolved
checker/checker.go Outdated Show resolved Hide resolved
checker/checker.go
@@ -75,14 +75,18 @@ type Checker struct {
sync.RWMutex
detail *check.Results
}
errCnt int64
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is it make sense to restrict the total length of the errors or the total numbers of error + warn instead of set two different count separately. e.g. if we think be default we only return 20 messages, then if errors + warns <= 20, we needn't truncate any messages.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I think error has a higher priority than warning, so if user gets 20 warnings and 1 errors we should make sure we will display that errors.

Copy link
Collaborator

Choose a reason for hiding this comment

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

we could loop the results twice to make sure errors have higher priority 🤔 I'm OK with current two settings, let's wait glorv

Copy link
Collaborator

Choose a reason for hiding this comment

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

I'm ok, too. I just wonder if this implementation is user-friendly enough to use since the goal is to solve the message too long issue.

BTW, I think maybe there are some cases the user just wants to emit the warning message, thus the --warn parameter can be used.

@GMHDBJD @glorv
Update checker/checker.go
0a41325 
Co-authored-by: glorv <glorvs@163.com>
@GMHDBJD @glorv
Update checker/checker.go
53153f9 
Co-authored-by: glorv <glorvs@163.com>
@lance6716
Copy link
Collaborator

/hold

@lance6716
Copy link
Collaborator

we now disable it by passing a big value instead of -1 🤔

@GMHDBJD
Copy link
Collaborator Author

GMHDBJD commented Apr 21, 2021

we now disable it by passing a big value instead of -1 🤔

cobra doesn't support negative argument... I keep the negative for other callings (like precheck of start-task). But now we use 10 as the default in every call, so I remove it.

@lance6716
Copy link
Collaborator

wait @glorv to cancel the hold

@lance6716
Copy link
Collaborator

DM-143 is failed because we didn't support optimistic sharding with PARTITION
/run-all-tests

@glorv
Copy link
Collaborator

glorv commented Apr 21, 2021

/unhold

@lance6716
Copy link
Collaborator

/run-all-tests

1 similar comment
@lance6716
Copy link
Collaborator

/run-all-tests

@ti-chi-bot ti-chi-bot merged commit 67b95c0 into pingcap:master Apr 22, 2021
ti-srebot pushed a commit to ti-srebot/dm that referenced this pull request Apr 22, 2021
@GMHDBJD @ti-srebot
cherry pick pingcap#1610 to release-2.0
ba3203f 
Signed-off-by: ti-srebot <ti-srebot@pingcap.com>
@ti-srebot ti-srebot mentioned this pull request Apr 22, 2021
Merged
@ti-srebot
Copy link

cherry pick to release-2.0 in PR #1621

@ti-srebot ti-srebot added already-cherry-pick-2.0 The related PR is already cherry-picked to release-2.0. Add this label once the PR is cherry-picked and removed 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 Apr 22, 2021
@lance6716 lance6716 added needs-update-docs Should update docs after this PR is merged. Remove this label once the docs are updated and removed needs-update-docs Should update docs after this PR is merged. Remove this label once the docs are updated labels Apr 22, 2021
ti-chi-bot pushed a commit that referenced this pull request Apr 22, 2021
@ti-srebot
check: add error/warn count for check-task (#1610) (#1621)
dce41f4
@lance6716 lance6716 mentioned this pull request Apr 24, 2021
Merged
@lance6716 lance6716 added the needs-update-docs Should update docs after this PR is merged. Remove this label once the docs are updated label May 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@lichunzhu lichunzhu lichunzhu left review comments

@glorv glorv glorv left review comments

@lance6716 lance6716 lance6716 approved these changes

Assignees
No one assigned
Labels
already-cherry-pick-2.0 The related PR is already cherry-picked to release-2.0. Add this label once the PR is cherry-picked needs-update-docs Should update docs after this PR is merged. Remove this label once the docs are updated size/XXL 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
Milestone
v2.0.3
Development

Successfully merging this pull request may close these issues.

grpc has a message limit, so long error message can't be received by dmctl
6 participants
@GMHDBJD @lichunzhu @ti-chi-bot @lance6716 @glorv @ti-srebot