New unified internal table names format: part 2, generating new names #15178

shlomi-noach · 2024-02-08T12:46:46Z

Description

This is the 2nd part of introducing the new vitess internal table names. This PR is post v19, and will be merged in v20.
In this PR, we still support both old name format and new name format. However, we now generate internal table names in new format only. This means:

GC table names will look like _vt_hld_6ace8bcef73211ea87e9f875a4d24e90_20200915120410_
OnlineDDL/Vreplication table names will look like _vt_vrp_6ace8bcef73211ea87e9f875a4d24e90_20200915120410_

Some implementation notes:

This PR renames functions in the likeness of "NameNewFormat" to "Name", and "Name" to "NameOldFormat".
Formalized InternalTableHint, which is also used in TableGCState
OnlineDDL/Executor previously generated the "_vrepl" table name by itself, directly; now it uses go/vt/schema to do so.
TabletManager/TableGC remains largely unaware of the change.
A few functions are not really used in production anymore, but are kept for the sake of existing testing. In v21 we will remove them.
gh-ost table names and pt-osc table names are unaffected by this change. gh-ost is a bit of a problem because there's no way right now to force it to accept the new naming format. We could update gh-ost, but then don't really have control over the gh-ost version the user chooses to install. Also, we're moving away from gh-ost and so it's not as important. This is a discussion for another issue.

Related Issue(s)

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

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

vitess-bot · 2024-02-08T12:46:49Z

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.

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

codecov · 2024-02-11T07:26:11Z

Codecov Report

Attention: 14 lines in your changes are missing coverage. Please review.

Comparison is base (67dcba5) 70.63% compared to head (ec2b976) 67.37%.
Report is 21 commits behind head on main.

Files	Patch %	Lines
go/vt/schema/tablegc.go	72.72%	6 Missing ⚠️
go/vt/vttablet/onlineddl/executor.go	0.00%	5 Missing ⚠️
go/vt/schema/name.go	86.95%	3 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #15178      +/-   ##
==========================================
- Coverage   70.63%   67.37%   -3.27%     
==========================================
  Files        1377     1560     +183     
  Lines      182801   192773    +9972     
==========================================
+ Hits       129127   129876     +749     
- Misses      53674    62897    +9223

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

shlomi-noach · 2024-02-11T07:47:29Z

Good to review.

mattlord

LGTM. I only had a few minor comments/notes/questions that you can resolve as you feel best.

mattlord · 2024-02-12T18:52:33Z

go/vt/schema/name.go

+// If uuid is given, then it must be in condensed-UUID format. If empty, the function auto-generates a UUID.
+func GenerateInternalTableName(hint string, uuid string, t time.Time) (tableName string, err error) {
+ if len(hint) != 3 {
+ return "", fmt.Errorf("Invalid hint: %s, expected 3 characters", hint)


Any reason not to use vterrors here so that we have codes? IIRC you added some error handling not too long ago that relied on error codes. Error messages also aren't supposed to be capitalized (for wrapping).

https://go.dev/wiki/CodeReviewComments#error-strings

mattlord · 2024-02-12T18:56:38Z

go/vt/schema/name_test.go

+ assert.NoError(t, err)
+
+ readableTimestamp := ToReadableTimestamp(ti)
+ assert.Equal(t, readableTimestamp, "20150225110639")


I think it would be more meaningful if we didn't use a string literal here but instead used ti here.

The point was to validate that a transformation of ti lands at an expected value. So that constant should remain IMO. I did, however, switch between readableTimestamp and "20150225110639" because the constant is the expected one.

mattlord · 2024-02-12T20:38:59Z

go/vt/schema/tablegc.go

+ switch s {
+ case HoldTableGCState:
+ return InternalTableGCHoldHint
+ case PurgeTableGCState:
+ return InternalTableGCPurgeHint
+ case EvacTableGCState:
+ return InternalTableGCEvacHint
+ case DropTableGCState:
+ return InternalTableGCDropHint
+ default:
+ return InternalTableUnknownHint
+ }


Any reason not to make a map of these states like protobuf does? Not a big deal, but the use of Internal (capital I) and Hint here are not obvious to the average reader (me).

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

…generating new names (#4400) * cherry pick of 15178 * updte mysql-server-private image ahead of planetscale/vitess-private#4435 Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> --------- Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com> Co-authored-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

New unified internal table names format: part 2, generating new names

078bb0f

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

shlomi-noach requested review from rohit-nayak-ps, mattlord and deepthi as code owners February 8, 2024 12:46

github-actions bot added this to the v20.0.0 milestone Feb 8, 2024

shlomi-noach marked this pull request as draft February 8, 2024 13:06

shlomi-noach added 5 commits February 11, 2024 08:47

restore functions used in endtoend test

f78e48f

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

check error code

56db8cf

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

use ReadableTimestamp()

4ca4459

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

more tests

34438a0

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

typo

f720936

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

shlomi-noach marked this pull request as ready for review February 11, 2024 07:47

shlomi-noach requested a review from a team February 11, 2024 07:47

dbussink approved these changes Feb 12, 2024

View reviewed changes

mattlord approved these changes Feb 12, 2024

View reviewed changes

states to hint: make it a map

ec2b976

Signed-off-by: Shlomi Noach <2607934+shlomi-noach@users.noreply.github.com>

shlomi-noach merged commit 09c3d56 into vitessio:main Feb 15, 2024
102 checks passed

shlomi-noach deleted the new-internal-table-names-post-19 branch February 15, 2024 05:43

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

New unified internal table names format: part 2, generating new names #15178

New unified internal table names format: part 2, generating new names #15178

shlomi-noach commented Feb 8, 2024 •

edited

Loading

vitess-bot bot commented Feb 8, 2024

codecov bot commented Feb 11, 2024 •

edited

Loading

shlomi-noach commented Feb 11, 2024

mattlord left a comment

mattlord Feb 12, 2024

mattlord Feb 12, 2024

mattlord Feb 12, 2024

shlomi-noach Feb 13, 2024

mattlord Feb 12, 2024

shlomi-noach Feb 13, 2024

New unified internal table names format: part 2, generating new names #15178

New unified internal table names format: part 2, generating new names #15178

Conversation

shlomi-noach commented Feb 8, 2024 • edited Loading

Description

Related Issue(s)

Checklist

Deployment Notes

vitess-bot bot commented Feb 8, 2024

Review Checklist

General

Tests

Documentation

New flags

If a workflow is added or modified:

Backward compatibility

codecov bot commented Feb 11, 2024 • edited Loading

Codecov Report

shlomi-noach commented Feb 11, 2024

mattlord left a comment

Choose a reason for hiding this comment

mattlord Feb 12, 2024

Choose a reason for hiding this comment

mattlord Feb 12, 2024

Choose a reason for hiding this comment

mattlord Feb 12, 2024

Choose a reason for hiding this comment

shlomi-noach Feb 13, 2024

Choose a reason for hiding this comment

mattlord Feb 12, 2024

Choose a reason for hiding this comment

shlomi-noach Feb 13, 2024

Choose a reason for hiding this comment

shlomi-noach commented Feb 8, 2024 •

edited

Loading

codecov bot commented Feb 11, 2024 •

edited

Loading