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

plugin: introduce plugin framework #8788

Merged
merged 9 commits into from Jan 14, 2019

Conversation

@lysu
Copy link
Member

commented Dec 24, 2018

What problem does this PR solve?

introduce plugin framework to tidb

What is changed and how it works?

How to use

here just a simple describe how to write a new plugin, later will give a full developer guide.

  • choose a plugin kind or no suitable need add new plugin kind
  • create a normal go package, add manifest.toml like example one
  • implement validate, init, destory method which is needed for all plugin
  • implement kind special method to implement plugin logic
  • use cmd/pluginpkg to build plugin binary, and put plugin binary into plugin deploy folder
  • start TiDB with -plugin-dir and -plugin-load parameter
  • show plugin to check it's load status

What change

add a new plugin pkg, the design detail described in a proposal doc

Check List

Tests

  • Unit test
  • Manual test (add detailed scripts or steps below)
  • WIP

Code changes

  • Add new package
  • Add new cmd tool

Side effects

  • Increased code complexity

Related changes

  • Need to update the documentation
  • Need to be included in the release note

Code Review Guide


This change is Reviewable

@lysu lysu changed the title plugin: introduce plugin framework to TiDB plugin: introduce plugin framework Dec 24, 2018
@lysu lysu added the status/WIP label Dec 24, 2018
@shenli

This comment has been minimized.

Copy link
Member

commented Dec 24, 2018

@lysu Good job!
This is a huge PR. Could you split it into multiple PRs? At least you can send the proposal first.

@jackysp jackysp added the type/TEP label Dec 25, 2018
Copy link
Contributor

left a comment

Would you split this PR into small parts?
For example, document and example can be a separate one;
show plugins and config related things could comes after the plugin package core.

Smaller PR is more friendly to reviewers.

cmd/pluginpkg/pluginpkg.go Outdated Show resolved Hide resolved
plugin/conn_ip_example/conn_ip_example.go Outdated Show resolved Hide resolved
plugin/const.go Outdated Show resolved Hide resolved
return
}
p := tiPlugins.plugins[kind][i]
if err = p.OnInit(ctx, p.Manifest); err != nil {

This comment has been minimized.

Copy link
@tiancaiamao

tiancaiamao Jan 9, 2019

Contributor

Should we defer panic for plugin init, thus plugin panic will not make TiDB panic ?

This comment has been minimized.

Copy link
@lysu

lysu Jan 10, 2019

Author Member

IMHO, it's better let framework user to decide panic or do other things - -

This comment has been minimized.

Copy link
@lysu

lysu Jan 10, 2019

Author Member

@tiancaiamao PR has be splitted.

plugin/plugin.go Show resolved Hide resolved
plugin/spi.go Show resolved Hide resolved
@lysu lysu force-pushed the lysu:dev-plugin-fw branch 2 times, most recently from c1ced25 to 3751c5a Jan 10, 2019
@lysu lysu force-pushed the lysu:dev-plugin-fw branch from 3751c5a to b7848f9 Jan 10, 2019
@lysu lysu requested a review from jackysp Jan 10, 2019
@lysu lysu added component/plugin and removed status/WIP labels Jan 10, 2019
@codecov-io

This comment has been minimized.

Copy link

commented Jan 10, 2019

Codecov Report

Merging #8788 into master will decrease coverage by 0.21%.
The diff coverage is 7.27%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #8788      +/-   ##
==========================================
- Coverage   67.63%   67.41%   -0.22%     
==========================================
  Files         364      370       +6     
  Lines       75471    75732     +261     
==========================================
+ Hits        51043    51057      +14     
- Misses      19929    20176     +247     
  Partials     4499     4499
Impacted Files Coverage Δ
plugin/const.go 0% <0%> (ø)
plugin/conn_ip_example/conn_ip_example.go 0% <0%> (ø)
plugin/plugin.go 0% <0%> (ø)
plugin/spi.go 100% <100%> (ø)
plugin/errors.go 100% <100%> (ø)
plugin/helper.go 12.5% <12.5%> (ø)
ddl/session_pool.go 82.75% <0%> (-10.35%) ⬇️
executor/join.go 77.92% <0%> (-0.52%) ⬇️
executor/distsql.go 72.53% <0%> (-0.47%) ⬇️
... and 6 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8ac79f3...cf8a45c. Read the comment docs.

plugin/plugin.go Show resolved Hide resolved
return errors.Errorf("plugin %s isn't loaded so can not reload", p.Name)
}
if len(p.SysVars) != len(oldPlugin.SysVars) {
return errors.Errorf("reload plugin with different sysVar is unsupported")

This comment has been minimized.

Copy link
@jackysp

jackysp Jan 11, 2019

Member

It's best to define a plugin error category that we will need it sooner or later.

This comment has been minimized.

Copy link
@lysu

lysu Jan 13, 2019

Author Member

it's little confuse, so we should make plugin rely on parser's predefine error?

This comment has been minimized.

Copy link
@lysu

lysu Jan 13, 2019

Author Member

@tiancaiamao @jackysp message move to parser, please review parser first pingcap/parser#173

plugin/conn_ip_example/conn_ip_example.go Outdated Show resolved Hide resolved
plugin/const.go Outdated Show resolved Hide resolved
plugin/helper.go Outdated Show resolved Hide resolved
type ID string

// Decode decodes a plugin id into name, version parts.
func (n ID) Decode() (name string, version string, err error) {

This comment has been minimized.

Copy link
@tiancaiamao

tiancaiamao Jan 12, 2019

Contributor

// Plugin name should not contain '-'

This comment has been minimized.

Copy link
@lysu

lysu Jan 13, 2019

Author Member

Decode has check a42f8e9#diff-51012a4c21599aa07955e2f374958792R40, refine msg in new commit, and pluginpkg also add a check 5bbc246

plugin/plugin.go Show resolved Hide resolved
plugin/plugin.go Show resolved Hide resolved
// ExportManifest exports a manifest to TiDB as a known format.
// it just casts sub-manifest to manifest.
func ExportManifest(m interface{}) *Manifest {
return (*Manifest)((*(*iface)(unsafe.Pointer(&m))).Data)

This comment has been minimized.

Copy link
@tiancaiamao

tiancaiamao Jan 12, 2019

Contributor

If Manifest is not the first field in the extended manifest struct, this code broke.

This comment has been minimized.

Copy link
@lysu

lysu Jan 13, 2019

Author Member

Yes, but only tidb developer can add new kind of manifest, this can be restricted by code review(and it also has be documented); for plugin develop only touch toml configuration with predefine manifest

plugin/spi.go Outdated Show resolved Hide resolved
plugin/spi_test.go Outdated Show resolved Hide resolved
@lysu lysu force-pushed the lysu:dev-plugin-fw branch from 310105d to 5bbc246 Jan 13, 2019
lysu added 2 commits Jan 13, 2019
@lysu lysu force-pushed the lysu:dev-plugin-fw branch from 5738419 to 04cd85f Jan 13, 2019
@lysu

This comment has been minimized.

Copy link
Member Author

commented Jan 13, 2019

/run-all-tests

@tiancaiamao

This comment has been minimized.

Copy link
Contributor

commented Jan 13, 2019

LGTM
Please fix CI

@lysu

This comment has been minimized.

Copy link
Member Author

commented Jan 13, 2019

ok~ @tiancaiamao now this PR use my parser branch so CI fail on tidy check, after parser PR shipped this will pass

Copy link
Member

left a comment

LGTM

@lysu

This comment has been minimized.

Copy link
Member Author

commented Jan 14, 2019

/run-all-tests

@lysu lysu added status/LGT2 and removed status/LGT1 labels Jan 14, 2019
@lysu lysu merged commit e1381b4 into pingcap:master Jan 14, 2019
12 of 13 checks passed
12 of 13 checks passed
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
ci/circleci Your tests passed on CircleCI!
Details
idc-jenkins-ci-tidb/build Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/check_dev Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/common-test job succeeded
Details
idc-jenkins-ci-tidb/integration-common-test Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/integration-compatibility-test Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/integration-ddl-test Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/mybatis-test job succeeded
Details
idc-jenkins-ci-tidb/sqllogic-test-1 Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/sqllogic-test-2 Jenkins job succeeded.
Details
idc-jenkins-ci-tidb/unit-test Jenkins job succeeded.
Details
license/cla Contributor License Agreement is signed.
Details
lysu added a commit to lysu/tidb that referenced this pull request Mar 25, 2019
# Conflicts:
#	cmd/pluginpkg/pluginpkg.go
#	go.mod
#	go.sum
lysu added a commit to lysu/tidb that referenced this pull request Mar 25, 2019
# Conflicts:
#	cmd/pluginpkg/pluginpkg.go
#	go.mod
#	go.sum
zz-jason added a commit that referenced this pull request Mar 25, 2019
@lysu lysu deleted the lysu:dev-plugin-fw branch Mar 25, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.