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

Add skeleton to initialize tso microservice #5917

Merged

Conversation

binshi-bing
Copy link
Contributor

@binshi-bing binshi-bing commented Feb 7, 2023

Issue Number: ref #5836

Signed-off-by: Bin Shi binshi.bing@gmail.com

What problem does this PR solve?

Issue Number: Ref #5836

What is changed and how does it work?

This PR added the skeleton along the E2E path to initialize the TSO microservice.
It mainly contains the following change:

1. Parse the subcommands from the command line arguments and refactor the entrypoints for all-in-one pd service and other independent microservice. Needs to compare with @lhy1024's recent PR #5858
2. Refactor Server/TSO config. 
3. Add the skeleton alone E2E path to initialize the TSO microservice.

Check List

Tests

  • Unit test
  • Manual test (add detailed scripts or steps below)
    I did some compatibility test manually by verifying that the all-in-one pd-server can start normally w/o and w/ parameter.

Code changes

  • Has configuration change

Related changes

Release note

None.

@ti-chi-bot
Copy link
Member

ti-chi-bot commented Feb 7, 2023

[REVIEW NOTIFICATION]

This pull request has been approved by:

  • lhy1024
  • rleungx

To complete the pull request process, please ask the reviewers in the list to review by filling /cc @reviewer in the comment.
After your PR has acquired the required number of LGTMs, you can assign this pull request to the committer in the list by filling /assign @committer in the comment to help you merge this pull request.

The full list of commands accepted by this bot can be found here.

Reviewer can indicate their review by submitting an approval review.
Reviewer can cancel approval by submitting a request changes review.

@ti-chi-bot ti-chi-bot added the release-note The PR should write the release note. label Feb 7, 2023
@ti-chi-bot
Copy link
Member

Hi @binshi-bing. Thanks for your PR.

I'm waiting for a tikv member to verify that this patch is reasonable to test. If it is, they should reply with /ok-to-test on its own line. Until that is done, I will not automatically test new commits in this PR, but the usual testing commands by org members will still work. Regular contributors should join the org to skip this step.

Once the patch is verified, the new status will be reflected by the ok-to-test label.

I understand the commands that are listed here.

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.

Makefile Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Feb 7, 2023

Codecov Report

Base: 75.35% // Head: 75.43% // Increases project coverage by +0.07% 🎉

Coverage data is based on head (cbd774e) compared to base (a3bb320).
Patch coverage: 83.33% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff             @@
##           master    #5917      +/-   ##
==========================================
+ Coverage   75.35%   75.43%   +0.07%     
==========================================
  Files         347      348       +1     
  Lines       35482    35486       +4     
==========================================
+ Hits        26739    26769      +30     
+ Misses       6406     6394      -12     
+ Partials     2337     2323      -14     
Flag Coverage Δ
unittests 75.43% <83.33%> (+0.07%) ⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
pkg/tso/config.go 0.00% <0.00%> (ø)
server/server.go 74.14% <ø> (-1.01%) ⬇️
pkg/utils/configutil/configutil.go 100.00% <100.00%> (ø)
server/config/config.go 77.61% <100.00%> (-0.88%) ⬇️
pkg/storage/endpoint/rule.go 83.33% <0.00%> (-11.12%) ⬇️
...erver/config/service_middleware_persist_options.go 91.66% <0.00%> (-8.34%) ⬇️
server/region_syncer/server.go 82.96% <0.00%> (-3.30%) ⬇️
server/schedule/labeler/labeler.go 76.77% <0.00%> (-2.59%) ⬇️
pkg/election/lease.go 84.05% <0.00%> (-1.45%) ⬇️
... and 20 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

pkg/basic_server/basic_server.go Outdated Show resolved Hide resolved
pkg/basic_server/basic_server.go Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
cmd/pd-server/main.go Outdated Show resolved Hide resolved
cmd/pd-server/main.go Outdated Show resolved Hide resolved
pkg/utils/configutil/configutil.go Outdated Show resolved Hide resolved
pkg/tso/allocator_manager.go Outdated Show resolved Hide resolved
pkg/basic_server/basic_server.go Show resolved Hide resolved
@ti-chi-bot ti-chi-bot added release-note-none and removed release-note The PR should write the release note. labels Feb 7, 2023
@binshi-bing binshi-bing closed this Feb 7, 2023
@binshi-bing binshi-bing force-pushed the initialize-tso-microservice-skeleton branch from 0a22f7e to 03081c6 Compare February 7, 2023 19:13
@binshi-bing
Copy link
Contributor Author

binshi-bing commented Feb 7, 2023

This PR is closed by accident. I did the following thing:

  1. git rebase master in my local branch.
  2. After resolving conflict, I tried to push it to remote branch
    ~/code/pingcap/my-pd  initialize-tso-microservice-skeleton  git push origin initialize-tso-microservice-skeleton
  3. the push was rejected with the following error message:
    error: failed to push some refs to 'github.com:binshi-bing/pd.git'
    hint: Updates were rejected because the tip of your current branch is behind
  4. On github page, I manually sync the origin/initialize-tso-microservice-skeleton with origin/master which disarded all my commits in the PR, so the PR was closed automatically.
  5. now rerunning step 2 succeeded, but the PR remains closed.
  6. I reopened the PR.

Questions:

  1. Am I supposed to run git push --force at step 2? Is `git push --force' the recommended way at this situation?

@binshi-bing binshi-bing reopened this Feb 7, 2023
@binshi-bing
Copy link
Contributor Author

OK, I can reopen it when I updated the previous comment. I'm still wondering what's the recommend way at step 2.

cmd/pd-server/main.go Outdated Show resolved Hide resolved
pkg/tso/config.go Outdated Show resolved Hide resolved
pkg/mcs/tso/server/server.go Outdated Show resolved Hide resolved
pkg/mcs/tso/server/server.go Outdated Show resolved Hide resolved
pkg/tso/config.go Outdated Show resolved Hide resolved
Copy link
Member

@rleungx rleungx left a comment

Choose a reason for hiding this comment

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

The rest LGTM

@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Feb 8, 2023
pkg/basic_server/basic_server.go Show resolved Hide resolved
pkg/mcs/tso/server/server.go Outdated Show resolved Hide resolved
server/server.go Show resolved Hide resolved
@binshi-bing binshi-bing force-pushed the initialize-tso-microservice-skeleton branch 2 times, most recently from 68f317a to fb09b95 Compare February 8, 2023 07:28
@ti-chi-bot ti-chi-bot added status/LGT2 Indicates that a PR has LGTM 2. and removed status/LGT1 Indicates that a PR has LGTM 1. labels Feb 8, 2023
@binshi-bing binshi-bing force-pushed the initialize-tso-microservice-skeleton branch from fb09b95 to 240a2a7 Compare February 8, 2023 16:13
Issue Number: ref tikv#5836

Signed-off-by: Bin Shi <binshi.bing@gmail.com>
Signed-off-by: Bin Shi <binshi.bing@gmail.com>
Signed-off-by: Bin Shi <binshi.bing@gmail.com>
Will integrate with @lhy1024's subcommand change later.

Handle feedback.

Signed-off-by: Bin Shi <binshi.bing@gmail.com>
@binshi-bing binshi-bing force-pushed the initialize-tso-microservice-skeleton branch from 240a2a7 to 6f5f084 Compare February 9, 2023 02:08
@rleungx
Copy link
Member

rleungx commented Feb 9, 2023

/merge

@ti-chi-bot
Copy link
Member

@rleungx: It seems you want to merge this PR, I will help you trigger all the tests:

/run-all-tests

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 ti-community-infra/tichi repository.

@ti-chi-bot
Copy link
Member

This pull request has been accepted and is ready to merge.

Commit hash: 6f5f084

@ti-chi-bot ti-chi-bot added the status/can-merge Indicates a PR has been approved by a committer. label Feb 9, 2023
@lhy1024
Copy link
Contributor

lhy1024 commented Feb 9, 2023

/ok-to-test

@lhy1024
Copy link
Contributor

lhy1024 commented Feb 9, 2023

/test build

@rleungx
Copy link
Member

rleungx commented Feb 9, 2023

/rebuild

@lhy1024
Copy link
Contributor

lhy1024 commented Feb 9, 2023

/retest

@ti-chi-bot
Copy link
Member

@binshi-bing: Your PR was out of date, I have automatically updated it for you.

If the CI test fails, you just re-trigger the test that failed and the bot will merge the PR for you after the CI passes.

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 ti-community-infra/tichi repository.

@ti-chi-bot ti-chi-bot merged commit 28a95cd into tikv:master Feb 9, 2023
@binshi-bing binshi-bing deleted the initialize-tso-microservice-skeleton branch February 14, 2023 17:11
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ok-to-test release-note-none status/can-merge Indicates a PR has been approved by a committer. status/LGT2 Indicates that a PR has LGTM 2.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants