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

clarify collations are also supported for db_charset #16423

Merged
merged 3 commits into from
Jul 30, 2024

Conversation

mcrauwel
Copy link
Contributor

Description

This is a documentation change to clarify the ability to configure a specific collation in case your environment runs different MySQL version where different character sets might have different collation, in which case replication might break

Related Issue(s)

Fixed #16422

Checklist

  • "Backport to:" labels have been added if this change should be back-ported to release branches
  • If this change is to be back-ported to previous releases, a justification is included in the PR description
  • Tests were added or are not required
  • Did the new or modified tests pass consistently locally and on CI?
  • Documentation was added or is not required

Deployment Notes

@mcrauwel mcrauwel requested a review from deepthi as a code owner July 17, 2024 15:56
Copy link
Contributor

vitess-bot bot commented Jul 17, 2024

Review Checklist

Hello reviewers! 👋 Please follow this checklist when reviewing this Pull Request.

General

  • Ensure that the Pull Request has a descriptive title.
  • Ensure there is a link to an issue (except for internal cleanup and flaky test fixes), new features should have an RFC that documents use cases and test cases.

Tests

  • Bug fixes should have at least one unit or end-to-end test, enhancement and new features should have a sufficient number of tests.

Documentation

  • Apply the release notes (needs details) label if users need to know about this change.
  • New features should be documented.
  • There should be some code comments as to why things are implemented the way they are.
  • There should be a comment at the top of each new or modified test to explain what the test does.

New flags

  • Is this flag really necessary?
  • Flag names must be clear and intuitive, use dashes (-), and have a clear help text.

If a workflow is added or modified:

  • Each item in Jobs should be named in order to mark it as required.
  • If the workflow needs to be marked as required, the maintainer team must be notified.

Backward compatibility

  • Protobuf changes should be wire-compatible.
  • Changes to _vt tables and RPCs need to be backward compatible.
  • RPC changes should be compatible with vitess-operator
  • If a flag is removed, then it should also be removed from vitess-operator and arewefastyet, if used there.
  • vtctl command output order should be stable and awk-able.

@vitess-bot vitess-bot bot added NeedsBackportReason If backport labels have been applied to a PR, a justification is required NeedsDescriptionUpdate The description is not clear or comprehensive enough, and needs work NeedsIssue A linked issue is missing for this Pull Request NeedsWebsiteDocsUpdate What it says labels Jul 17, 2024
@github-actions github-actions bot added this to the v21.0.0 milestone Jul 17, 2024
@mcrauwel mcrauwel force-pushed the mcrauwel/fix-16422 branch from 526e87f to bc91a8e Compare July 17, 2024 15:57
Signed-off-by: Matthias Crauwels <matthias.crauwels@planetscale.com>
@mcrauwel mcrauwel force-pushed the mcrauwel/fix-16422 branch from bc91a8e to 0b304bf Compare July 17, 2024 15:59
Copy link

codecov bot commented Jul 17, 2024

Codecov Report

Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

Project coverage is 68.61%. Comparing base (fab9071) to head (f090948).
Report is 3 commits behind head on main.

Files Patch % Lines
go/vt/dbconfigs/dbconfigs.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main   #16423      +/-   ##
==========================================
- Coverage   68.63%   68.61%   -0.02%     
==========================================
  Files        1551     1551              
  Lines      199515   199515              
==========================================
- Hits       136938   136907      -31     
- Misses      62577    62608      +31     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@deepthi deepthi left a comment

Choose a reason for hiding this comment

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

Looks fine to me. The endtoend tests need to be fixed, they capture flag help output.
Most likely just vttablet.txt, but the test failure will tell you.

@deepthi deepthi added Component: VTTablet Type: Documentation and removed NeedsDescriptionUpdate The description is not clear or comprehensive enough, and needs work NeedsWebsiteDocsUpdate What it says NeedsIssue A linked issue is missing for this Pull Request NeedsBackportReason If backport labels have been applied to a PR, a justification is required labels Jul 17, 2024
@deepthi
Copy link
Member

deepthi commented Jul 17, 2024

Most likely just vttablet.txt, but the test failure will tell you.

There are 5 binaries which are affected, so the 5 corresponding files need to change under go/flags/endtoend.

Copy link
Contributor

@shlomi-noach shlomi-noach left a comment

Choose a reason for hiding this comment

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

🌷

Signed-off-by: Matthias Crauwels <matthias.crauwels@planetscale.com>
@@ -40,7 +40,7 @@ Flags:
--db-credentials-vault-tls-ca string Path to CA PEM for validating Vault server certificate
--db-credentials-vault-tokenfile string Path to file containing Vault auth token; token can also be passed using VAULT_TOKEN environment variable
--db-credentials-vault-ttl duration How long to cache DB credentials from the Vault server (default 30m0s)
--db_charset string Character set used for this tablet. (default "utf8mb4")
--db_charset string Character set/collation used for this tablet. Make sure to configure this to a charset/collation supported by the lowest MySQL version in your environment. (default "utf8mb4")
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 probably want to drop the Make sure bit since for anyone running MySQL 8 and later (which is the only supported version), the default is fine and doesn't need configuring.

Copy link
Member

Choose a reason for hiding this comment

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

What are you proposing? Drop the entire second sentence? Can you post that as a suggestion so that it is clearer?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dbussink FWIW, I kept the wording vague not mentioning $oldUnsupportedVersion. Just for the case that oracle would add a newer version of the Unicode character set in a future major upgrade… In that case this would become relevant again… but if the consensus is to remove the sentence, I can do that…

Copy link
Contributor

Choose a reason for hiding this comment

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

I'd say we can drop the entire second sentence? Maybe something like:

Character set or collation name used by this tablet for the connection to MySQL

Just for the case that oracle would add a newer version of the Unicode character set in a future major upgrade…

I doubt this would happen, but they do have 4 chances to do this 😉. See:

SELECT id, COLLATION_NAME FROM INFORMATION_SCHEMA.COLLATIONS order by id;
...
| 249 | gb18030_bin                 |
| 250 | gb18030_unicode_520_ci      |
| 255 | utf8mb4_0900_ai_ci          |
...

Giving this is a one byte field in the MySQL handshake protocol, only 251, 252, 253 & 254 are not used yet. You can't use any of the names with an Id > 255 here anyway because of that limitation.

But I also think that if that time comes, we can further clarify it with the situation that is then current?

Copy link
Member

Choose a reason for hiding this comment

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

I say let's merge this for now and if and when things change we can revisit.

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>
@shlomi-noach shlomi-noach merged commit 67b5a6d into vitessio:main Jul 30, 2024
129 checks passed
@shlomi-noach shlomi-noach deleted the mcrauwel/fix-16422 branch July 30, 2024 14:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature Request: clarify flag documentation for --db_charset
4 participants