Skip to content

Add loadbalancer documentation#5555

Closed
dveeden wants to merge 1 commit into
pingcap:masterfrom
dveeden:lb
Closed

Add loadbalancer documentation#5555
dveeden wants to merge 1 commit into
pingcap:masterfrom
dveeden:lb

Conversation

@dveeden
Copy link
Copy Markdown
Contributor

@dveeden dveeden commented Apr 29, 2021

What is changed, added or deleted? (Required)

Add documentation about how to use loadbalancers with TiDB

Which TiDB version(s) do your changes apply to? (Required)

  • master (the latest development version)
  • v5.0 (TiDB 5.0 versions)
  • v4.0 (TiDB 4.0 versions)
  • v3.1 (TiDB 3.1 versions)
  • v3.0 (TiDB 3.0 versions)
  • v2.1 (TiDB 2.1 versions)

@ti-chi-bot ti-chi-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Apr 29, 2021
@ti-chi-bot ti-chi-bot requested a review from TomShawn April 29, 2021 16:51
@ti-chi-bot ti-chi-bot added missing-translation-status This PR does not have translation status info. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 29, 2021
@ti-chi-bot ti-chi-bot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Apr 30, 2021
@dveeden dveeden force-pushed the lb branch 2 times, most recently from 1e0b0ae to e3ca504 Compare May 4, 2021 08:35
@ti-chi-bot ti-chi-bot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels May 4, 2021
@dveeden dveeden marked this pull request as ready for review May 4, 2021 08:48
@ti-chi-bot ti-chi-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label May 4, 2021
@dveeden dveeden force-pushed the lb branch 2 times, most recently from cda1411 to b4577f8 Compare May 4, 2021 09:49
@dveeden
Copy link
Copy Markdown
Contributor Author

dveeden commented May 4, 2021

@dveeden
Copy link
Copy Markdown
Contributor Author

dveeden commented May 4, 2021

We should consider merging or linking this with https://docs.pingcap.com/tidb/stable/haproxy-best-practices

@TomShawn
Copy link
Copy Markdown
Contributor

TomShawn commented May 6, 2021

/label needs-cherry-pick-5.0
/translation doing
/assign @Liuxiaozhen12
@dveeden Could you please involve technical review(s)? Thanks!

@ti-chi-bot ti-chi-bot added translation/doing This PR's assignee is translating this PR. needs-cherry-pick-5.0 and removed missing-translation-status This PR does not have translation status info. labels May 6, 2021
@TomShawn
Copy link
Copy Markdown
Contributor

TomShawn commented May 6, 2021

We could link this to https://docs.pingcap.com/tidb/stable/java-app-best-practices

You can link this to these docs in this PR.

Comment thread loadbalancing.md Outdated
@morgo
Copy link
Copy Markdown
Contributor

morgo commented Jul 27, 2021

/merge

@ti-chi-bot
Copy link
Copy Markdown
Member

@morgo: /merge in this pull request requires 2 approval(s).

Details

In response to this:

/merge

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.

@dveeden
Copy link
Copy Markdown
Contributor Author

dveeden commented Jul 28, 2021

@TomShawn PTAL

Comment thread loadbalancing.md Outdated

The first type of loadbalancing that is available is *connector based loadbalancing*. Many of the MySQL connectors like MySQL Connector/J and MySQL Connector/C++. The benefits from this are that there is no extra network hop and the application has more information about which server it is connected to. The drawback of this is that there is no central administration and when changing the configuration you have to do this on all your application hosts. Depending on the programming language you are using the loadbalancing may not be available or not offer more advanced options. This can work with most third-party applications as this is often configured in the connection string (JDBC url for Java for example).

It is also possible to use DNS round-robin. However this is not advisable as this won't prevent connections from going to a machine that is unavailable. This means that your application may have to re-try connections more often if not all servers are available.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

If this is not advisable, then we should not document it here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

As DNS-RR is a common practice I think we should explain here why other solutions are better.

@morgo what's your take on this?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I agree with @dveeden , it's very common and users need to know why we don't recommend it.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

OK, let's keep it.

@TomShawn
Copy link
Copy Markdown
Contributor

TomShawn commented Jul 28, 2021

@dveeden This is really a great article with a comprehensive introduction to loadbalancing. These are my general comments. PTAL for reference.

Some part of it, to me, looks like a blog post article that uses plain passages than a user document which usually prefers listing things out in unordered/ordered lists or tables (in a really structured layout). For example, the "Loadbalancing types" section introduces several types. It might be better to organize the information of this passage using a table, which makes your information clearer and more readable to readers.

Loadbalancing types User scenarios Benefits Drawbacks
Connector-based loadbalancing, such as MySQL Connector/J and MySQL Connector/C++ This can work with most third-party applications because this is often configured in the connection string (JDBC url for Java for example). There is no extra network hop and the application has more information about which server it is connected to. There is no central administration. When changing the configuration on one host, you have to do this on all your application hosts.
DNS round-robin

Another question is the target audience. To me, this can be confusing. I think it would be better to clarify what this article introduces and its aim at the very beginning. Then readers will get a general idea of the article from the beginning and decide whether or not to go on reading.

  • If it is about the best practices of loadbalancing in TiDB, then maybe we should not introduce too much about the loadbalancing types (including the benefit and drawback, even for the unrecommended types). And its TOC position can also be changed. "Best Practices" might be a better place for it.
  • If it is about a task-oriented instruction/guide for TiDB maintenance, then maybe many of the concept introductions look unnecessary and can be removed.

Then it is about the language style issue. In user documentation, we usually follow these styles:

  • Avoid using the first person (I/me/we/us/our/let's). Mostly, use "You" or passive voice.
  • Avoid abbreviations (let's, don't, e.g., etc...)
  • Avoid using "may" because it is ambiguous. Use "might" instead.
  • Make sentences as short and concise as possible.
  • Briefly introduce/summarize each section in one or two sentences at the beginning of the section.

@dveeden dveeden marked this pull request as draft July 29, 2021 08:05
@ti-chi-bot ti-chi-bot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jul 29, 2021
Co-authored-by: Morgan Tocker <tocker@gmail.com>
Co-authored-by: bb7133 <bb7133@gmail.com>
@dveeden
Copy link
Copy Markdown
Contributor Author

dveeden commented Aug 10, 2021

@TomShawn I went over your comments.

For the loadbalancer types I don't know if we should:

  • Use a table (as you suggested). This gets quite long
  • Use paragraphs (as the original had)
  • Use a table an paragraphs (current state) This avoids a table with very high/long cells.
  • Use subsections per type

What do you think?

@dveeden
Copy link
Copy Markdown
Contributor Author

dveeden commented Aug 10, 2021

In some places (pingcap/tidb, pingcap/docs, pingcap/tidb-operator) the solution with envoy an istio is used. However I don't think that should be part of this PR as that's mostly a TiDB Operator thing.

@dveeden dveeden marked this pull request as ready for review August 23, 2021 06:18
@ti-chi-bot ti-chi-bot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Aug 23, 2021
@dveeden dveeden requested a review from TomShawn August 23, 2021 06:18
@morgo morgo mentioned this pull request Aug 25, 2021
12 tasks
@TomShawn TomShawn added sig/docs Indicates that the Issue or PR belongs to the docs SIG. area/sql-infra Indicates that the Issue or PR belongs to the area of sql-infra and sql-metadata. labels Sep 6, 2021
@dveeden
Copy link
Copy Markdown
Contributor Author

dveeden commented Nov 22, 2021

This has been published as a blog instead of as part of the documentation.

@dveeden dveeden closed this Nov 22, 2021
@ti-chi-bot ti-chi-bot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Nov 22, 2021
@ti-chi-bot
Copy link
Copy Markdown
Member

@dveeden: PR needs rebase.

Details

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.

@TomShawn TomShawn removed type/enhancement The issue or PR belongs to an enhancement. translation/doing This PR's assignee is translating this PR. status/LGT1 Indicates that a PR has LGTM 1. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. sig/docs Indicates that the Issue or PR belongs to the docs SIG. area/sql-infra Indicates that the Issue or PR belongs to the area of sql-infra and sql-metadata. labels Nov 23, 2021
@ti-chi-bot ti-chi-bot added the status/LGT1 Indicates that a PR has LGTM 1. label Nov 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

status/LGT1 Indicates that a PR has LGTM 1.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants