VTAdmin: Support for schema migrations view/create #17134

beingnoble03 · 2024-11-04T10:36:52Z

Description

This PR adds a screen for schema migrations list for a particular keyspace, and a screen to create schema migration request. Also includes some related API changes.

Screenshots

Related Issue(s)

VTAdmin: Enhance VReplication functionality in vtadmin UI #11690

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: Noble Mittal <noblemittal@outlook.com>

vitess-bot · 2024-11-04T10:36:55Z

codecov · 2024-11-04T11:31:10Z

Codecov Report

Attention: Patch coverage is 43.33333% with 17 lines in your changes missing coverage. Please review.

Project coverage is 67.41%. Comparing base (4980d6f) to head (3db89f7).
Report is 32 commits behind head on main.

Files with missing lines	Patch %	Lines
go/vt/vtadmin/http/schema_migrations.go	0.00%	11 Missing ⚠️
go/vt/vtadmin/api.go	68.42%	6 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main   #17134      +/-   ##
==========================================
+ Coverage   67.39%   67.41%   +0.02%     
==========================================
  Files        1570     1574       +4     
  Lines      252892   253247     +355     
==========================================
+ Hits       170437   170734     +297     
- Misses      82455    82513      +58

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

Signed-off-by: Noble Mittal <noblemittal@outlook.com>

rohit-nayak-ps · 2024-11-05T08:25:16Z

This UI is only for the vitess ddl strategy. So we don't need that option while creating.

I will let @shlomi-noach comment on the UI requirements for a first cut. We will also need a detail screen from the list view.

shlomi-noach · 2024-11-05T13:12:54Z

Hey there! A few comments:

DDL strategy can include DDL strategy flags. So for example it could look like vitess --postpone-completion --singleton-context --cut-over-threshold=15s. In light of that, let leave it editable. But then, it also requires quite a length box. I'd say keyspace has too much space, and DDL strategy too little.
It is possible to submit a migration with direct DDL strategy -- but then it won't show in the schema migrations page.
However, the strategy mysql is a hybrid of the two - and will show in the schema migrations page.

So yes, let's allow free text in the DDL strategy flag.

beingnoble03 · 2024-11-07T11:55:24Z

As discussed, I've removed the ddl-strategy text box, to allow only vitess ddl strategy. We can add the advanced options in a follow-up PR?

Signed-off-by: Noble Mittal <noblemittal@outlook.com>

shlomi-noach · 2024-11-07T12:40:46Z

As discussed, I've removed the ddl-strategy text box, to allow only vitess ddl strategy.

This looks to be a misunderstanding, in #17134 (comment) I indicated that we should keep it after all?

rohit-nayak-ps · 2024-11-08T08:55:42Z

As discussed, I've removed the ddl-strategy text box, to allow only vitess ddl strategy.

This looks to be a misunderstanding, in #17134 (comment) I indicated that we should keep it after all?

My bad. The plan was to create an initial PR with defaults and then add UI for the strategy options. But maybe we should just add the text box to start with, so that we do support everything and then replace them with the UI later. @beingnoble03, can you please reinstate the text box, sorry for the confusion.

shlomi-noach · 2024-11-08T09:27:41Z

@rohit-nayak-ps no problem, feel free to lead the way. I'll set some time next week to think about what we discussed over zoom, and how the UI can take shape in the next iteration.

Signed-off-by: Noble Mittal <noblemittal@outlook.com>

beingnoble03 · 2024-11-08T09:57:18Z

@rohit-nayak-ps @shlomi-noach done, also increased the size of the box as mentioned.

rohit-nayak-ps · 2024-11-11T16:35:27Z

On creating a new migration, it goes to the Migrations screen but without choosing a keyspace, so it shows up empty and is confusing. Also on refreshing it goes back to the empty screen because it doesn't remember the chosen keyspace.
We should also have a details page for each migration with more details including the actual sql query.
i created a migration in a sharded keyspace and it only shows one of the shards in the detail.
Also I think we should change the default strategy to vitess: since that is how we expect most users to run migrations. Remove gh-ost (already unsupported) and pt-osc (slated to be removed soon): https://vitess.io/docs/22.0/user-guides/schema-changes/ddl-strategies/.
Regarding UI: can we align the timestamps so that it is easier to compare.

Signed-off-by: Noble Mittal <noblemittal@outlook.com>

beingnoble03 · 2024-11-13T17:34:21Z

On creating a new migration, it goes to the Migrations screen but without choosing a keyspace, so it shows up empty and is confusing. Also on refreshing it goes back to the empty screen because it doesn't remember the chosen keyspace.

Fixed.

We should also have a details page for each migration with more details including the actual sql query.

Can this be done in follow-up PRs?

i created a migration in a sharded keyspace and it only shows one of the shards in the detail.

This is how it shows migrations for resharded keyspace. It shows different entries for different shards, even for the same migration. This is the format we get from GetSchemaMigrations (different objs for different shards). Please let me know if we can to club this together in UI, we can do this in a follow-up PR.

Also I think we should change the default strategy to vitess: since that is how we expect most users to run migrations. Remove gh-ost (already unsupported) and pt-osc (slated to be removed soon): https://vitess.io/docs/22.0/user-guides/schema-changes/ddl-strategies/.

Done.

Regarding UI: can we align the timestamps so that it is easier to compare.

Done.

Signed-off-by: Noble Mittal <noblemittal@outlook.com>

rohit-nayak-ps

Nice. Let's should do the following in a followup:

Show a single line for each migration, keyed with UUID. That line can show just the shards involved and a consolidated status (similar to how we show a combined Workflow status for all streams)
Add a details page where all the shards involved along with more info like the actual query itself is shown

notfelineit · 2024-11-26T21:55:16Z

web/vtadmin/src/components/routes/createSchemaMigration/CreateSchemaMigration.tsx

+
+    const history = useHistory();
+
+    const [formData, setFormData] = useState<FormData>(DEFAULT_FORM_DATA);


This implementation of the form fields will cause the entire page to rerender whenever a parameter of formData changes, since below, we set formData to a brand new object each time.

Would prefer using flat state vars or simply using the HTML form data in a submission - however this is a nonblocking comment as you/we can refactor in a separate PR! 😄

👍 I think this makes more sense to be done in a separate PR, as there are other components using the same structure.

notfelineit · 2024-11-26T21:55:31Z

web/vtadmin/src/util/schemaMigrations.ts

+import { vtctldata } from '../proto/vtadmin';
+
+/**
+ * SCEMA_MIGRATION_STATUS maps numeric schema migration status back to human readable strings.


Suggested change

* SCEMA_MIGRATION_STATUS maps numeric schema migration status back to human readable strings.

* SCHEMA_MIGRATION_STATUS maps numeric schema migration status back to human readable strings.

notfelineit · 2024-11-26T21:55:38Z

web/vtadmin/src/util/schemaMigrations.ts

+/**
+ * SCEMA_MIGRATION_STATUS maps numeric schema migration status back to human readable strings.
+ */
+export const SCEMA_MIGRATION_STATUS = Object.entries(invertBy(vtctldata.SchemaMigration.Status)).reduce(


Suggested change

export const SCEMA_MIGRATION_STATUS = Object.entries(invertBy(vtctldata.SchemaMigration.Status)).reduce(

export const SCHEMA_MIGRATION_STATUS = Object.entries(invertBy(vtctldata.SchemaMigration.Status)).reduce(

notfelineit · 2024-11-26T21:55:51Z

web/vtadmin/src/util/schemaMigrations.ts

+);
+
+export const formatSchemaMigrationStatus = (schemaMigration: vtctldata.ISchemaMigration) =>
+    schemaMigration.status && SCEMA_MIGRATION_STATUS[schemaMigration.status];


Suggested change

schemaMigration.status && SCEMA_MIGRATION_STATUS[schemaMigration.status];

schemaMigration.status && SCHEMA_MIGRATION_STATUS[schemaMigration.status];

Signed-off-by: Noble Mittal <noblemittal@outlook.com>

VTAdmin: Support for schema migrations view/create

ca76dc7

Signed-off-by: Noble Mittal <noblemittal@outlook.com>

github-actions bot added this to the v22.0.0 milestone Nov 4, 2024

beingnoble03 force-pushed the vtadmin-schema-migrations branch from cdbf4bc to f27341a Compare November 4, 2024 14:41

refac: Move error_dialog to common components

ddc420a

Signed-off-by: Noble Mittal <noblemittal@outlook.com>

beingnoble03 force-pushed the vtadmin-schema-migrations branch from f27341a to ddc420a Compare November 4, 2024 14:45

beingnoble03 marked this pull request as ready for review November 4, 2024 14:48

beingnoble03 requested review from deepthi, harshit-gangal, mattlord, ajm188, notfelineit and rohit-nayak-ps as code owners November 4, 2024 14:48

Remove the ddl-strategy text box

3b0d9f2

Signed-off-by: Noble Mittal <noblemittal@outlook.com>

beingnoble03 added 2 commits November 8, 2024 15:13

revert: Bring back the text-box

f9511d9

Signed-off-by: Noble Mittal <noblemittal@outlook.com>

Increase size of the strategy box

2875bd7

Signed-off-by: Noble Mittal <noblemittal@outlook.com>

Merge branch 'main' into vtadmin-schema-migrations

ee0b786

Signed-off-by: Noble Mittal <noblemittal@outlook.com>

Use URLParams for selecting the keyspace and fix alignment

fefdb5d

Signed-off-by: Noble Mittal <noblemittal@outlook.com>

beingnoble03 force-pushed the vtadmin-schema-migrations branch from 7ad0af7 to fefdb5d Compare November 13, 2024 17:35

rohit-nayak-ps approved these changes Nov 18, 2024

View reviewed changes

rohit-nayak-ps requested review from notfelineit and removed request for notfelineit November 19, 2024 18:57

deepthi requested review from shlomi-noach and removed request for harshit-gangal, ajm188 and shlomi-noach November 25, 2024 18:25

notfelineit reviewed Nov 26, 2024

View reviewed changes

notfelineit approved these changes Nov 26, 2024

View reviewed changes

Fix typo

3db89f7

Signed-off-by: Noble Mittal <noblemittal@outlook.com>

rohit-nayak-ps merged commit 68b25b3 into vitessio:main Nov 29, 2024
102 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VTAdmin: Support for schema migrations view/create #17134

VTAdmin: Support for schema migrations view/create #17134

beingnoble03 commented Nov 4, 2024 •

edited

Loading

vitess-bot bot commented Nov 4, 2024

codecov bot commented Nov 4, 2024 •

edited

Loading

rohit-nayak-ps commented Nov 5, 2024

shlomi-noach commented Nov 5, 2024

beingnoble03 commented Nov 7, 2024

shlomi-noach commented Nov 7, 2024

rohit-nayak-ps commented Nov 8, 2024

shlomi-noach commented Nov 8, 2024

beingnoble03 commented Nov 8, 2024

rohit-nayak-ps commented Nov 11, 2024

beingnoble03 commented Nov 13, 2024 •

edited

Loading

rohit-nayak-ps left a comment

notfelineit Nov 26, 2024

beingnoble03 Nov 27, 2024

notfelineit Nov 26, 2024

notfelineit Nov 26, 2024

notfelineit Nov 26, 2024


		const history = useHistory();

		const [formData, setFormData] = useState<FormData>(DEFAULT_FORM_DATA);

	* SCEMA_MIGRATION_STATUS maps numeric schema migration status back to human readable strings.
	* SCHEMA_MIGRATION_STATUS maps numeric schema migration status back to human readable strings.

	export const SCEMA_MIGRATION_STATUS = Object.entries(invertBy(vtctldata.SchemaMigration.Status)).reduce(
	export const SCHEMA_MIGRATION_STATUS = Object.entries(invertBy(vtctldata.SchemaMigration.Status)).reduce(

	schemaMigration.status && SCEMA_MIGRATION_STATUS[schemaMigration.status];
	schemaMigration.status && SCHEMA_MIGRATION_STATUS[schemaMigration.status];

VTAdmin: Support for schema migrations view/create #17134

VTAdmin: Support for schema migrations view/create #17134

Conversation

beingnoble03 commented Nov 4, 2024 • edited Loading

Description

Screenshots

Related Issue(s)

Checklist

Deployment Notes

vitess-bot bot commented Nov 4, 2024

Review Checklist

General

Tests

Documentation

New flags

If a workflow is added or modified:

Backward compatibility

codecov bot commented Nov 4, 2024 • edited Loading

Codecov Report

rohit-nayak-ps commented Nov 5, 2024

shlomi-noach commented Nov 5, 2024

beingnoble03 commented Nov 7, 2024

shlomi-noach commented Nov 7, 2024

rohit-nayak-ps commented Nov 8, 2024

shlomi-noach commented Nov 8, 2024

beingnoble03 commented Nov 8, 2024

rohit-nayak-ps commented Nov 11, 2024

beingnoble03 commented Nov 13, 2024 • edited Loading

rohit-nayak-ps left a comment

Choose a reason for hiding this comment

notfelineit Nov 26, 2024

Choose a reason for hiding this comment

beingnoble03 Nov 27, 2024

Choose a reason for hiding this comment

notfelineit Nov 26, 2024

Choose a reason for hiding this comment

notfelineit Nov 26, 2024

Choose a reason for hiding this comment

notfelineit Nov 26, 2024

Choose a reason for hiding this comment

beingnoble03 commented Nov 4, 2024 •

edited

Loading

codecov bot commented Nov 4, 2024 •

edited

Loading

beingnoble03 commented Nov 13, 2024 •

edited

Loading