Skip to content
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

store TiDB server info to PD and add http api handle #7082

Merged
merged 55 commits into from
Aug 15, 2018

Conversation

crazycs520
Copy link
Contributor

@crazycs520 crazycs520 commented Jul 18, 2018

What have you changed? (mandatory)

This PR is for issue#6961

  1. Store TiDB server static information to PD when DDL server start up and delete when server down. The information will not update when TiDB is running.So I call this static information.
  2. Add 2 http-api.
    • {TiDBIP}:10080/info request for TiDBIP server information.
    • {TiDBIP}:10080/info/all requSchemaSyncerest for all TiDB server information.
  3. static information
ID         string `json:"ddl_id"`
IP         string `json:"ip"`
Port       uint   `json:"listening_port"`
StatusPort uint   `json:"status_port"`
Lease      string `json:"lease"`
Version string `json:"version"`
GitHash string `json:"git_hash"`

What is the type of the changes? (mandatory)

  • New feature (non-breaking change which adds functionality)

How has this PR been tested? (mandatory)

add in tidb-test. https://github.com/pingcap/tidb-test/pull/593

Does this PR affect documentation (docs/docs-cn) update? (mandatory)

Yes

Does this PR affect tidb-ansible update? (mandatory)

Yes

Does this PR need to be added to the release notes? (mandatory)

Store TiDB information to PD and add http-api to get TiDB and cluster information.

Refer to a related PR or issue link (optional)

Benchmark result if necessary (optional)

Add a few positive/negative examples (optional)

@winkyao
Copy link
Contributor

winkyao commented Jul 18, 2018

Does this PR need to be added to the release notes? (mandatory) YES

@winkyao winkyao added the release-note Denotes a PR that will be considered when it comes time to generate release notes. label Jul 18, 2018
ddl/ddl.go Outdated
@@ -510,6 +525,46 @@ func (d *ddl) SetBinlogClient(binlogCli interface{}) {
d.binlogCli = binlogCli
}

func (d *ddl) GetServerInfo() *util.DDLServerInfo {
Copy link
Contributor

Choose a reason for hiding this comment

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

You could write it like this.

func (d *ddl) GetServerInfo() *util.DDLServerInfo {
	cfg := config.GetGlobalConfig()
	info := &util.DDLServerInfo{
		ID:         d.uuid,
		IP:         cfg.AdvertiseAddress,
		StatusPort: cfg.Status.StatusPort,
		Lease:      cfg.Lease,
	}
	info.Version = mysql.ServerVersion
	info.GitHash = printer.TiDBGitHash
	return info
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

@crazycs520
Copy link
Contributor Author

@winkyao @ciscoxll @zimulala PTAL

ddl/ddl.go Outdated
@@ -207,6 +209,15 @@ type DDL interface {

// SetBinlogClient sets the binlog client for DDL worker. It's exported for testing.
SetBinlogClient(interface{})

// GetServerInfo get self DDL server static information.
GetServerInfo() *util.DDLServerInfo
Copy link
Member

Choose a reason for hiding this comment

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

This is not related with DDL.

Copy link
Contributor

@XuHuaiyu XuHuaiyu left a comment

Choose a reason for hiding this comment

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

Documentation should be updated?

ddl/ddl.go Outdated
@@ -376,6 +387,10 @@ func (d *ddl) close() {
if err != nil {
log.Errorf("[ddl] remove self version path failed %v", err)
}
err = d.schemaSyncer.RemoveSelfServerInfo()
if err != nil {
log.Errorf("[ddl] remove self server info path failed %v", err)
Copy link
Contributor

Choose a reason for hiding this comment

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

remove path?

ddl/syncer.go Outdated
@@ -20,11 +20,14 @@ import (
"sync"
"time"

"encoding/json"
Copy link
Contributor

Choose a reason for hiding this comment

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

put this line to line 17

ddl/syncer.go Outdated
time.Sleep(200 * time.Millisecond)
continue
}
if err == nil && len(resp.Kvs) > 0 {
Copy link
Contributor

Choose a reason for hiding this comment

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

no need to check err == nil here

ddl/syncer.go Outdated
err := json.Unmarshal(resp.Kvs[0].Value, info)
if err != nil {
log.Infof("[syncer] get ddl server info, ddl %s json.Unmarshal %v failed %v.", resp.Kvs[0].Key, resp.Kvs[0].Value, err)
return nil, err
Copy link
Contributor

Choose a reason for hiding this comment

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

errors.Trace(err)

ddl/syncer.go Outdated
err := json.Unmarshal(kv.Value, info)
if err != nil {
log.Infof("[syncer] get all ddl server info, ddl %s json.Unmarshal %v failed %v.", kv.Key, kv.Value, err)
return nil, err
Copy link
Contributor

Choose a reason for hiding this comment

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

errors.Trace(err)

@XuHuaiyu
Copy link
Contributor

Maybe it's better to specify what static information does this PR store to PD.

ddl/syncer.go Outdated
// GetAllServerInfoFromPD get all DDL servers information from PD.
GetAllServerInfoFromPD(ctx context.Context) (map[string]*util.DDLServerInfo, error)
// UpdateSelfServerInfo store DDL server information to PD.
UpdateSelfServerInfo(ctx context.Context, info *util.DDLServerInfo) error
Copy link
Contributor

Choose a reason for hiding this comment

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

Since the info will not be updated during running,
I think that name it as StoreSelfServerInfoToPD is enough

@shenli
Copy link
Member

shenli commented Jul 22, 2018

@crazycs520 Please address the comments.

domain/info.go Outdated
}
info, ok := infoMap[id]
if !ok {
return nil, errors.Errorf("[infoSyncer] get %s failed", key)
Copy link
Contributor

Choose a reason for hiding this comment

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

I think "info-syncer" is better.

domain/info.go Outdated
return errors.Trace(is.newSessionAndStoreServerInfo(ctx, owner.NewSessionDefaultRetryCnt))
}

//GetServerInfo gets self server static information.
Copy link
Contributor

Choose a reason for hiding this comment

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

s//Get// Get

log.Error(err)
return
}
ownerID, err := do.DDL().OwnerManager().GetOwnerID(ctx)
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need to add a timeout to ctx?

Copy link
Contributor

@zimulala zimulala left a comment

Choose a reason for hiding this comment

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

LGTM

@zimulala zimulala added status/LGT2 Indicates that a PR has LGTM 2. component/DDL-need-LGT3 and removed component/DDL-need-LGT3 status/LGT1 Indicates that a PR has LGTM 1. labels Aug 13, 2018
ddl/syncer.go Outdated
@@ -83,6 +83,8 @@ type SchemaSyncer interface {
// the latest schema version. If the result is false, wait for a while and check again util the processing time reach 2 * lease.
// It returns until all servers' versions are equal to the latest version or the ctx is done.
OwnerCheckAllVersions(ctx context.Context, latestVer int64) error
// GetSession return the session lease id of SchemaSyncer.
GetSessionLeaseID() clientv3.LeaseID
Copy link
Member

Choose a reason for hiding this comment

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

s/GetSessionLeaseID/GetLeaseID/

domain/info.go Outdated
// InfoSessionTTL is the etcd session's TTL in seconds. It's exported for testing.
var InfoSessionTTL = 10 * 60

// InfoSyncer stores server info to etcd when when the tidb-server starts and delete when tidb-server shuts down.
Copy link
Member

Choose a reason for hiding this comment

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

when when -> when

domain/info.go Outdated
}

// ServerInfo is server static information.
// It will not update when server running. So please only put static information in ServerInfo struct.
Copy link
Member

Choose a reason for hiding this comment

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

s/update/be updated/ or change
when server running -> during tidb-server process lifetime

StatusPort: cfg.Status.StatusPort,
Lease: cfg.Lease,
}
info.Version = mysql.ServerVersion
Copy link
Member

Choose a reason for hiding this comment

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

Why not move the two lines into the above block?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Version and GitHash belong to ServerVersionInfo. ServerInfo contain ServerVersionInfo with no attribute name.

ddl/syncer.go Outdated
func (s *schemaVersionSyncer) putKV(ctx context.Context, retryCnt int, key, val string,
// PutKVToEtcd puts key value to etcd.
// etcdCli is client of etcd.
// retryCnt is retry time when When an error occurs.
Copy link
Contributor

Choose a reason for hiding this comment

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

s/ when when / when

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done.

domain/info.go Outdated
)

// InfoSessionTTL is the etcd session's TTL in seconds. It's exported for testing.
var InfoSessionTTL = 10 * 60
Copy link
Member

Choose a reason for hiding this comment

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

1 mins is long enough.

domain/info.go Outdated

// Restart restart the info syncer with new session leaseID and store server info to etcd again.
func (is *InfoSyncer) Restart(ctx context.Context) error {
return errors.Trace(is.newSessionAndStoreServerInfo(ctx, owner.NewSessionRetryUnlimited))
Copy link
Member

Choose a reason for hiding this comment

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

Why use NewSessionRetryUnlimited?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I see in schemaVersionSyncer. Restart() use NewSessionRetryUnlimited when Restart, so...
How about your option?

@crazycs520
Copy link
Contributor Author

@shenli PTAL

Copy link
Contributor

@ciscoxll ciscoxll left a comment

Choose a reason for hiding this comment

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

LGTM

ciscoxll
ciscoxll previously approved these changes Aug 14, 2018
@crazycs520
Copy link
Contributor Author

/run-all-tests

1 similar comment
@shenli
Copy link
Member

shenli commented Aug 15, 2018

/run-all-tests

@crazycs520
Copy link
Contributor Author

/run-all-tests

@crazycs520 crazycs520 merged commit 9fc67b9 into pingcap:master Aug 15, 2018
AstroProfundis added a commit to AstroProfundis/tidb-insight that referenced this pull request Aug 20, 2018
ethercflow pushed a commit to pingcap/tidb-insight that referenced this pull request Aug 21, 2018
* tidb: add info collected from TiDB's server API

 * See pingcap/tidb#7082 for details of the API

* tidb: minor cleanup of pdctl code

* tidb: check wheather tidb server api is supported

* tidb: fix runtime error when collecting pdctl
@you06 you06 added the sig/sql-infra SIG: SQL Infra label Mar 4, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/sql-infra SIG: SQL Infra status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants