ddl: refine some context usage #56243
Conversation
Signed-off-by: lance6716 <lance6716@gmail.com>
ti-chi-bot
bot
added
release-note-none
|
Hi @lance6716. Thanks for your PR. PRs from untrusted users cannot be marked as trusted with I understand the commands that are listed here. 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 kubernetes-sigs/prow repository. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #56243 +/- ##
================================================
+ Coverage 73.4215% 76.3608% +2.9392%
================================================
Files 1619 1648 +29
Lines 446764 462300 +15536
================================================
+ Hits 328021 353016 +24995
+ Misses 98638 87718 -10920
- Partials 20105 21566 +1461
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Signed-off-by: lance6716 <lance6716@gmail.com>
ti-chi-bot
bot
added
size/L
ti-chi-bot
bot
added
size/XL
lance6716
changed the title
ti-chi-bot
bot
removed
the
do-not-merge/work-in-progress
lance6716
changed the title
lance6716
changed the title
Signed-off-by: lance6716 <lance6716@gmail.com>
Signed-off-by: lance6716 <lance6716@gmail.com>
pkg/ddl/partition.go
Outdated
| @@ -2738,7 +2738,7 @@ func (w *worker) onExchangeTablePartition(jobCtx *jobContext, t *meta.Meta, job | |||
| zap.Stringer("job", job), zap.Int64("defID", defID), zap.Int64("partDef.ID", partDef.ID)) | |||
| job.Args[0] = partDef.ID | |||
| defID = partDef.ID | |||
| err = updateDDLJob2Table(w.sess, job, true) | |||
| err = updateDDLJob2Table(w.ctx, w.sess, job, true) | |||
There was a problem hiding this comment.
use context from jobCtx
below too
| return &OperatorCtx{ | ||
| Context: opCtx, | ||
| cancel: cancel, | ||
| } | ||
| }, cancel |
There was a problem hiding this comment.
why not just use OperatorCtx.Cancel?
There was a problem hiding this comment.
the unused variable (cancel) can remind the caller. This PR fixes some functions that forget to cancel, like line 678 in pkg/ddl/backfilling.go in the old code.
There was a problem hiding this comment.
I doubt the remind part, coders can simple ignore it as OperatorCtx already has it
We should unify the place to call cancel, not scattering it all around
There was a problem hiding this comment.
The developers have forgotten to call OperatorCtx.Cancel at line 678 in pkg/ddl/backfilling.go (there's also a missing in unit test files), so I prefer to use golang's built-in detect of unused variables to prevent future bug.
There was a problem hiding this comment.
static checks doesn't help those logic bugs, but ok anyway
Signed-off-by: lance6716 <lance6716@gmail.com>
This comment was marked as outdated.
This comment was marked as outdated.
ti-chi-bot
bot
added
the
do-not-merge/hold
lance6716
changed the title
ti-chi-bot
bot
added
the
do-not-merge/work-in-progress
Signed-off-by: lance6716 <lance6716@gmail.com>
Signed-off-by: lance6716 <lance6716@gmail.com>
lance6716
changed the title
ti-chi-bot
bot
removed
the
do-not-merge/work-in-progress
|
/unhold |
ti-chi-bot
bot
removed
the
do-not-merge/hold
ti-chi-bot
bot
added
lgtm
and removed
needs-1-more-lgtm
|
/retest |
|
@lance6716: Cannot trigger testing until a trusted user reviews the PR and leaves an 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 kubernetes-sigs/prow repository. |
|
/retest |
|
@lance6716: Cannot trigger testing until a trusted user reviews the PR and leaves an 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 kubernetes-sigs/prow repository. |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: D3Hunter, tangenta, YuJuncen The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
|
@lance6716: The following test failed, say
Full PR test history. Your PR dashboard. 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 kubernetes/test-infra repository. I understand the commands that are listed here. |
|
/retest |
|
@lance6716: Cannot trigger testing until a trusted user reviews the PR and leaves an 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 kubernetes-sigs/prow repository. |
ti-chi-bot
bot
added
the
needs-cherry-pick-release-7.5
Signed-off-by: ti-chi-bot <ti-community-prow-bot@tidb.io>
|
In response to a cherrypick label: new pull request created to branch |
ti-chi-bot
bot
removed
the
needs-cherry-pick-release-7.5
What problem does this PR solve?
Issue Number: ref #56017 ref #54436
Problem Summary:
What changed and how does it work?
changed some functions to use correct context.
Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.