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.

Sign up for GitHub

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

Jump to bottom

refactor so that using the PD API avoids unnecessary imports #618

Merged
merged 7 commits into from
Jul 3, 2019
Merged

refactor so that using the PD API avoids unnecessary imports #618

merged 7 commits into from
Jul 3, 2019

Conversation

gregwebs
Copy link
Contributor

@gregwebs gregwebs commented Jul 2, 2019 •
edited
Loading

The PD API no longer imports Kubernetes and the CRD.

In the create-statefulset-immediately PR this reduced the new wait-for-pd binary
by ~20M

Tests

  • Existing tests pass

Code changes

  • Has Go code change

Does this PR introduce a user-facing change?:

No

@gregwebs
refactor so that using the PD API avoids unecessary imports
4b9a2ed 
PD API no longer imports Kubernetes and the CRD.

In the create-statefulset-immediately PR this reduced the binary
by ~20M
@gregwebs gregwebs requested review from cofyc and tennix July 2, 2019 00:37
This was referenced Jul 2, 2019
Closed
Closed
@tennix
Copy link
Member

tennix commented Jul 2, 2019

image
I recompile all the binaries and didn't see the size reduce.

@gregwebs
Copy link
Contributor Author

gregwebs commented Jul 2, 2019

@tennix the size reduction can only be seen on the create-statefulset-immediately PR for the wait-for-pd binary. It was requested that I make this a separate PR. We could use this same technique for the discovery service and see it there also.

tennix
tennix previously approved these changes Jul 2, 2019
View reviewed changes
Copy link
Member

@tennix tennix left a comment

Choose a reason for hiding this comment

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

LGTM

@tennix
Copy link
Member

tennix commented Jul 2, 2019

@weekface PTAL

@weekface
Copy link
Contributor

weekface commented Jul 3, 2019 •
edited
Loading

the pkg/apis is the directory for K8s apis, so it is not a good idea to put pdapis here. @cofyc think so too: #581 (comment)

@gregwebs
Copy link
Contributor Author

gregwebs commented Jul 3, 2019

What directory should it be placed it? @weekface

@weekface
Copy link
Contributor

weekface commented Jul 3, 2019

Any other directories other than pkg/apis are fine, for example: pkg/pdapi.

@gregwebs
Copy link
Contributor Author

gregwebs commented Jul 3, 2019

@weekface okay, directories are moved. If you come up with a better idea feel free to move or name them something else. rg -l -i 'pkg/apis/pdapi' | xargs -i@ sed -i 's|pkg/apis/pdapi|pkg/pdapi|g' @

weekface
weekface previously approved these changes Jul 3, 2019
View reviewed changes
Copy link
Contributor

@weekface weekface left a comment

Choose a reason for hiding this comment

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

lgtm

@weekface
Copy link
Contributor

weekface commented Jul 3, 2019

/run-e2e-tests

@weekface
Copy link
Contributor

weekface commented Jul 3, 2019

@onlymellb @cofyc @xiaojingchen PTAL

@xiaojingchen
Copy link
Contributor

/run-e2e-tests

@gregwebs gregwebs dismissed stale reviews from weekface and tennix via 0022a08 July 3, 2019 14:48
@gregwebs gregwebs force-pushed the create-statefulset-immediately-pd-api branch 2 times, most recently from 549138a to 0022a08 Compare July 3, 2019 19:20
@gregwebs
Copy link
Contributor Author

gregwebs commented Jul 3, 2019

/run-e2e-tests

@gregwebs
Copy link
Contributor Author

gregwebs commented Jul 3, 2019

/run-e2e-tests

@gregwebs gregwebs merged commit af4b67d into pingcap:master Jul 3, 2019
@gregwebs gregwebs deleted the create-statefulset-immediately-pd-api branch July 3, 2019 21:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tennix tennix tennix left review comments

@weekface weekface weekface left review comments

@cofyc cofyc Awaiting requested review from cofyc

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@gregwebs @tennix @weekface @xiaojingchen