*: Update hasSysDB using check ID instead of Name, and support all other types. #43823
Conversation
|
[REVIEW NOTIFICATION] This pull request has not been approved. To complete the pull request process, please ask the reviewers in the list to review by filling The full list of commands accepted by this bot can be found here. Reviewer can indicate their review by submitting an approval review. |
ti-chi-bot
bot
added
release-note-none
do-not-merge/needs-triage-completed
size/XL
zimulala
force-pushed
the
zimuxia/check-sys
branch
from
93f0a24
to
0523e01
Compare
session/mock_bootstrap.go
Outdated
| // Make test faster. | ||
| if intest.InTest { | ||
| time.Sleep(5 * time.Millisecond) | ||
| } else { | ||
| time.Sleep(30 * time.Millisecond) | ||
| } |
There was a problem hiding this comment.
I think the whole function mockUpgradeToVerLatest is only used in tests?
There was a problem hiding this comment.
No，we also use it in IT
There was a problem hiding this comment.
Could you please update the comment to indicate that the new one is for unit test?
| @@ -369,32 +369,41 @@ func (d *ddl) addBatchDDLJobs2Table(tasks []*limitJobTask) error { | |||
| startTS = txn.StartTS() | |||
| return nil | |||
| }) | |||
| if err == nil { | |||
There was a problem hiding this comment.
This file only update two place.
One is here, make meet err != nil return directly.
ddl/ddl_worker.go
Outdated
| setJobStateToQueueing(job) | ||
|
|
||
| if d.stateSyncer.IsUpgradingState() { | ||
| hasSysDB, err := util.HasSysDB(job) |
There was a problem hiding this comment.
Another is check the HasSysDB's error.
There was a problem hiding this comment.
Maybe wrap this block into a separate function trySetJobToPausing() or setJobPausingForUpgrade()?
zimulala
force-pushed
the
zimuxia/check-sys
branch
from
00636f9
to
8b81f28
Compare
zimulala
force-pushed
the
zimuxia/check-sys
branch
from
8b81f28
to
90c17e4
Compare
| // HasSysDB checks if it has a system database. | ||
| func HasSysDB(job *model.Job) (bool, error) { | ||
| // TODO: Handle for the schema ID is 0, like ActionCreatePlacementPolicy. | ||
| switch job.Type { |
There was a problem hiding this comment.
I wonder if it's really necessary to enumerate all the possible job types. Can't we determine this by the job.SchemaID or job.SchemaName?
|
PR needs rebase. 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. |
ti-chi-bot
bot
added
the
needs-rebase
|
@zimulala: 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. |
|
@zimulala: The following test failed, say
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. |
What problem does this PR solve?
Issue Number: close #43818
Problem Summary:
What is changed and how it works?
hasSysDBusing check ID instead of Name, and support all other types.IsPausedBySystemwithIsPauseBySystem.Check List
Tests
Side effects
Documentation
Release note
Please refer to Release Notes Language Style Guide to write a quality release note.