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

*: remove tcp_port for tiflash >= 7.1.0 #2220

Merged
merged 4 commits into from
Jun 16, 2023
Merged

*: remove tcp_port for tiflash >= 7.1.0 #2220

merged 4 commits into from
Jun 16, 2023

Conversation

zanmato1984
Copy link
Contributor

@zanmato1984 zanmato1984 commented Jun 16, 2023 •
edited
Loading

What problem does this PR solve?

Remove tiflash tcp_port config for tiflash >= 7.1.0.

What is changed and how it works?

  1. For playground, remove tcp_port for tiflash < 7.1.0, as there are no compatibility and instance identity issues.
  2. For ansibile and cluster, not render tcp_port config in tiflash config file for tiflash >= 7.1.0.

Check List

Tests

  • No code

Code changes

  • Has exported function/method change

Side effects

  • None

Related changes

  • None

Release notes:

NONE

@ti-chi-bot ti-chi-bot bot requested review from nexustar and srstack June 16, 2023 08:39
@ti-chi-bot ti-chi-bot bot added the size/M Denotes a PR that changes 30-99 lines, ignoring generated files. label Jun 16, 2023
Lloyd-Pottiger
Lloyd-Pottiger reviewed Jun 16, 2023
View reviewed changes
components/playground/instance/tiflash_pre7.go Outdated
@@ -146,7 +146,7 @@ func (inst *TiFlashInstance) checkConfigOld(deployDir, clusterManagerPath string
}()

// Write default config to buffer
if err := writeTiFlashConfigOld(flashBuf, version, inst.TCPPort, inst.Port, inst.ServicePort, inst.StatusPort,
if err := writeTiFlashConfigOld(flashBuf, version, inst.Port, inst.ServicePort, inst.StatusPort,
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 we can not remove tcp port before tiflash 7.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Right, it'll cause single-node multi-instance conflict on 9000 port. Thanks for reminding.

Addressed.

@codecov-commenter
Copy link

codecov-commenter commented Jun 16, 2023 •
edited
Loading

Codecov Report

Patch coverage: 77.78% and no project coverage change.

Comparison is base (c3a7e31) 50.48% compared to head (c789c8b) 50.49%.

❗ Current head c789c8b differs from pull request most recent head 15d9d72. Consider uploading reports for the commit 15d9d72 to get more accurate results

Additional details and impacted files 
@@           Coverage Diff           @@
##           master    #2220   +/-   ##
=======================================
  Coverage   50.48%   50.49%           
=======================================
  Files         320      320           
  Lines       33671    33678    +7     
=======================================
+ Hits        16998    17003    +5     
- Misses      14621    14623    +2     
  Partials     2052     2052
Flag Coverage Δ
tiup 15.94% <0.00%> (-0.01%) ⬇️
unittest 22.68% <100.00%> (+0.01%) ⬆️

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

Impacted Files Coverage Δ
components/playground/instance/tiflash.go 43.33% <ø> (ø)
components/playground/instance/tiflash_pre7.go 41.88% <ø> (ø)
...ponents/playground/instance/tiflash_pre7_config.go 50.00% <ø> (ø)
pkg/tidbver/tidbver.go 89.47% <0.00%> (-3.25%) ⬇️
pkg/cluster/spec/tiflash.go 67.75% <100.00%> (+0.25%) ⬆️

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

@ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented Jun 16, 2023

@Lloyd-Pottiger: adding LGTM is restricted to approvers and reviewers in OWNERS files.

In response to this:

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.

@kaaaaaaang
Copy link
Collaborator

/approve

@ti-chi-bot ti-chi-bot bot added the approved label Jun 16, 2023
@kaaaaaaang
Copy link
Collaborator

/lgtm

@ti-chi-bot ti-chi-bot bot added the lgtm label Jun 16, 2023
@ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented Jun 16, 2023

[LGTM Timeline notifier]

Timeline:

  • 2023-06-16 10:31:23.228626881 +0000 UTC m=+340879.643230955: ☑️ agreed by kaaaaaaang.

@ti-chi-bot
Copy link
Contributor

ti-chi-bot bot commented Jun 16, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: kaaaaaaang, Lloyd-Pottiger

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

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ti-chi-bot ti-chi-bot bot merged commit c2ae2f6 into pingcap:master Jun 16, 2023
18 checks passed
@kaaaaaaang kaaaaaaang added this to the 1.12.4 milestone Jul 10, 2023
kaaaaaaang pushed a commit that referenced this pull request Jul 13, 2023
@zanmato1984 @nexustar
*: remove tcp_port for tiflash >= 7.1.0 (#2220)
4fd37f0
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Lloyd-Pottiger Lloyd-Pottiger Lloyd-Pottiger approved these changes

@nexustar nexustar Awaiting requested review from nexustar

@srstack srstack Awaiting requested review from srstack

Assignees
No one assigned
Labels
approved lgtm size/M Denotes a PR that changes 30-99 lines, ignoring generated files.
Projects
None yet
Milestone
1.12.4
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@zanmato1984 @codecov-commenter @kaaaaaaang @Lloyd-Pottiger