-
Notifications
You must be signed in to change notification settings - Fork 5.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
session: add no register API for using TiDB as a library #17513
Conversation
No release note, Please follow https://github.com/pingcap/community/blob/master/contributors/release-note-checker.md |
Codecov Report
@@ Coverage Diff @@
## master #17513 +/- ##
===========================================
Coverage 79.3272% 79.3272%
===========================================
Files 541 541
Lines 145636 145636
===========================================
Hits 115529 115529
Misses 20812 20812
Partials 9295 9295 |
@breeswish PTAL, or... Please add some labels... |
When BR is integrated into TiDB, we don't need this PR, right? I think the integration is very close. |
domain/domain.go
Outdated
@@ -655,6 +655,14 @@ func NewDomain(store kv.Storage, ddlLease time.Duration, statsLease time.Duratio | |||
|
|||
// Init initializes a domain. | |||
func (do *Domain) Init(ddlLease time.Duration, sysFactory func(*Domain) (pools.Resource, error)) error { | |||
if err := do.InitWithNoRegister(ddlLease, sysFactory); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This also disables the DDL syncer, will it cause problems?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems InfoSyncer
won't do things about DDL... Is there something I ignored...?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know how BR uses the DDL structure. If BR uses InitWithNoRegister
to new a DDL, then the DDL also will campaign DDL owner, but it will make load schema slower then Init
with InfoSyncer
. Is it OK?
domain/infosync/info.go
Outdated
@@ -149,6 +149,9 @@ func (is *InfoSyncer) init(ctx context.Context) error { | |||
if err != nil { | |||
return err | |||
} | |||
if config.GetGlobalConfig().NoRegister { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we'd better to use config.GetGlobalConfig().NoRegister
as a argument to GlobalInfoSyncerInit
. It is best to reduce the strong correlation between internal objects and config.GetGlobalConfig()
.
/run-all-tests |
That is strange... It fails but I cannot find any useful information about why it fails... Let me retry it. /run-all-tests |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
@zimulala,Thanks for your review. |
@breeswish PTAL~ |
/run-all-tests
...Why? 🤔 |
/run-common-test
.... |
/run-common-test
We died at different place each time 🙃... |
/run-unit-test |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/merge |
/merge |
/run-all-tests |
* domain,session: add NoRegister entry * session: run fmt * session: remove old NoRegister API * config: add no register config * config: add example to example config file. * session: remove unused type * *: rename NoRegister to SkipRegisterToDashboard Co-authored-by: crazycs <crazycs520@gmail.com>
What problem does this PR solve?
Issue Number: close br#299
Problem Summary:
BR uses TiDB as a library, but currently when we call
session.GetDomain
,domap.Get
will automatically register itself to PD. Then a strange TiDB will present at dashboard.see more at br#299.
What is changed and how it works?
What's Changed:
I added a config option
no-register
to TiDB.Then, when this is enabled,
topologySyncer
won't be started, hence this TiDB won't register itself to dashboard.How it Works:
We add a if-guard to where initializing
topologySyncer
atinfoSyncer.Init
andSession.Init
.Check List
Tests
no-register = true
, then BR won't register it self as a TiDB anymore.More things
Maybe we have more things to do, for now please add
WIP
tag for this:Add test cases.(IMO, this is too hard to be tested automatically.)Don't make(We can setNoRegister
ed TiDB become DDL owner.run-DDL = false
)Release note