Skip to content

feat(1-1-restore): set tombstone_gc mode to 'repair' for views #4377

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

Merged

Conversation

VAveryanov8
Copy link
Collaborator

@VAveryanov8 VAveryanov8 commented May 8, 2025

This includes views into alterTGC stage as view is a normal table under the hood.

Refs: #4261

P.S. For now it's not clear what the behavior of Secondary Indexes:

  1. we don't know if the tombstone_gc mode is inherited from base table. DESC statement doesn't show options for indexes, see Describe schema with internals is missing Index options scylladb#22849
  2. It looks like there is no way to provide tombstone_gc mode during index creation or any other properties Add syntax for create a secondary index with synchronous view updates scylladb#16454

Please make sure that:

  • Code is split to commits that address a single change
  • Commit messages are informative
  • Commit titles have module prefix
  • Commit titles have issue nr. suffix

@VAveryanov8 VAveryanov8 self-assigned this May 8, 2025
@VAveryanov8 VAveryanov8 marked this pull request as ready for review May 8, 2025 10:21
Copy link
Collaborator

@Michal-Leszczynski Michal-Leszczynski left a comment

Choose a reason for hiding this comment

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

Let's also wait for a response to asked question.

@VAveryanov8 VAveryanov8 requested a review from Copilot May 12, 2025 09:37
Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates the tombstone_gc mode functionality by setting it to "repair" for views along with tables. Key changes include refactoring the view retrieval logic in worker_views.go to use a schema lookup, updating tombstone_gc mode functions in worker_tgc.go to handle both tables and views, and enhancing the integration tests to validate these changes.

Reviewed Changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated no comments.

File Description
pkg/service/one2onerestore/worker_views.go Refactored view retrieval to use a described schema map and removed dummy metadata creation
pkg/service/one2onerestore/worker_tgc.go Introduced unified functions for getting and setting tombstone_gc mode that handle both tables and views
pkg/service/one2onerestore/service_integration_test.go Updated integration tests to validate tombstone_gc mode for both materialized views and secondary indexes (with secondary index check commented)
Comments suppressed due to low confidence (2)

pkg/service/one2onerestore/worker_views.go:94

  • [nitpick] Consider using a more descriptive variable or helper function to construct the key for secondary indexes. This can help clarify that the '_index' suffix is intentionally added to differentiate secondary indexes from regular tables or views.
stmt, ok := describedViews[scyllaTable{keyspace: index.KeyspaceName, table: index.Name + "_index"}]

pkg/service/one2onerestore/worker_tgc.go:77

  • [nitpick] The parameter 'name' could be renamed to something like 'objectName' or 'tableOrViewName' for clarity, reflecting that this function handles both tables and views.
func (w *worker) getTombstoneGCMode(keyspace, name string, isView bool) (tombstoneGCMode, error) {

@VAveryanov8 VAveryanov8 force-pushed the va/1-1-restore-views-set-tgc-repair branch from 9ff1611 to ad94dda Compare May 15, 2025 09:18
This includes views into alterTGC stage as view is a normal table
under the hood.

Refs: #4261
This changes the way information about views is obtained - it's more
reliable to use DESC SCHEMA output, then information from the gocql
driver.
@VAveryanov8 VAveryanov8 force-pushed the va/1-1-restore-views-set-tgc-repair branch from 0a7b5be to f95a600 Compare June 2, 2025 10:07
Copy link
Collaborator

@Michal-Leszczynski Michal-Leszczynski left a comment

Choose a reason for hiding this comment

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

This is not a part of this PR, but still important:

	views, err := w.dropViews(ctx, workload)
	if err != nil {
		return errors.Wrap(err, "drop views")
	}
	defer func() {
		if rErr := w.reCreateViews(ctx, views); rErr != nil {
			err = multierr.Combine(
				err,
				errors.Wrap(rErr, "recreate views"),
			)
		}
	}()

	return w.restoreTables(ctx, workload, target.Keyspace)

Why are we recreating views even if restore tables failed?
Is this some sort of cleanup after failed restore? If so, then it probably should be handled with more care, because if SM restored table partially and the failed, we don't want to wait for the view recreation to finish (such view would need to be re-dropped and re-re-created again anyway). Generally speaking, handling the state of the restored cluster in case of failure is a little bit problematic, so it's okay to just say that the cluster needs to be "nuked" (schema-wise) and restore started from scratch for now.

Comment on lines 91 to 104
func (w *worker) getBaseTableName(keyspace, name string) (string, error) {
q := qb.Select("system_schema.views").
Columns("base_table_name").
Where(qb.Eq("keyspace_name"), qb.Eq("view_name")).
Query(w.clusterSession).
Bind(keyspace, name)
defer q.Release()
var baseTableName string
err := q.Scan(&baseTableName)
if err != nil {
return "", errors.Wrapf(err, "scan base table name for view %s.%s", keyspace, name)
}

return views, nil
return baseTableName, nil
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit: it should be more efficient to query all tuples of keyspace_name, view_name, base_table_name at once.

This changes the behavior in case of restoreTables return error,
previously we would attempt to recreateViews, but without paying
attention to the fact that base tables can have some (partial) data.
@VAveryanov8 VAveryanov8 merged commit b6e6d39 into 1-1-restore-feature-branch Jun 4, 2025
41 checks passed
@VAveryanov8 VAveryanov8 deleted the va/1-1-restore-views-set-tgc-repair branch June 4, 2025 10:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants