Skip to content

refactor(1-1-restore): refactors 1-1-restore progress #4348

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

Open
wants to merge 3 commits into
base: 1-1-restore-feature-branch
Choose a base branch
from

Conversation

VAveryanov8
Copy link
Collaborator

@VAveryanov8 VAveryanov8 commented Apr 14, 2025

This refactors 1-1-restore progress to address code review comments from #4296 by making following changes:

  • include nodetool refresh into table progress
  • only track progress of table/view restore
  • removes unnecessary stats (shard_count, host bandwidth, aggregation by
    host, keyspaces). These stats are better to keep in metrics or
    calculate on the client side.

Now getProgress api response will have following structure:

{
  "tables": [
    {
      "started_at": "2025-01-01T12:00:01Z",
      "completed_at": "2025-01-01T12:00:03Z",
      "size": 20,
      "restored": 20,
      "status": "done",
      "keyspace": "ks1",
      "table": "t2"
    },
    {
      "started_at": "2025-01-01T12:00:00Z",
      "completed_at": null,
      "size": 40,
      "restored": 40,
      "status": "in_progress",
      "keyspace": "ks1",
      "table": "t1"
    }
  ],
  "views": [
    {
      "started_at": "2025-01-01T12:00:01Z",
      "completed_at": null,
      "size": 0,
      "restored": 0,
      "status": "in_progress",
      "keyspace": "ks1",
      "table": "v1",
      "type": "MaterializedView"
    },
    {
      "started_at": "2025-01-01T12:01:03Z",
      "completed_at": "2025-01-01T12:00:04Z",
      "size": 0,
      "restored": 0,
      "status": "done",
      "keyspace": "ks1",
      "table": "v2",
      "type": "MaterializedView"
    }
  ]
}

status field is introduced to better cover cases when we can't say if restore operation of table/view is complete just by looking at size and restored, because for tables we need to wait until nodetool refresh is complete and for views - until view rebuild operation is complete.

Refs: #4205


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 force-pushed the va/1-1-restore-progress-refactor branch from 8c15d92 to 04922ec Compare April 14, 2025 14:56
@karol-kokoszka karol-kokoszka force-pushed the 1-1-restore-feature-branch branch from c5bf399 to f10f9f7 Compare April 14, 2025 18:00
@karol-kokoszka
Copy link
Collaborator

karol-kokoszka commented Apr 14, 2025

@VAveryanov8 please pull from the feature branch (or git stash your commits before, and git stash pop after pulling them)

@karol-kokoszka karol-kokoszka force-pushed the 1-1-restore-feature-branch branch from f10f9f7 to d664493 Compare April 14, 2025 19:33
This refactors 1-1-restore progress by making following changes:
- include `nodetool refresh` into table progress
- only track progress of table/view restore
- removes unnecessary stats (shard_count, host bandwidth, aggregation by
  host, keyspaces). These stats is better to keep in metrics or
  calculate on the client side.

  Refs: #4205
@VAveryanov8 VAveryanov8 force-pushed the va/1-1-restore-progress-refactor branch 3 times, most recently from b4d31b3 to ca6e046 Compare April 15, 2025 13:36
This introduces "failed" status for the ProgressStatus, which is used
only if table/view restore ended with error.
@VAveryanov8 VAveryanov8 force-pushed the va/1-1-restore-progress-refactor branch from ca6e046 to c2f8135 Compare April 15, 2025 13:41
@VAveryanov8 VAveryanov8 self-assigned this Apr 15, 2025
@VAveryanov8 VAveryanov8 marked this pull request as ready for review April 15, 2025 14:36
@VAveryanov8 VAveryanov8 requested a review from Copilot April 17, 2025 13:24
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.

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

Files not reviewed (12)
  • pkg/service/one2onerestore/testdata/1.run_table_progress.json: Language not supported
  • pkg/service/one2onerestore/testdata/1.run_view_progress.json: Language not supported
  • pkg/service/one2onerestore/testdata/2.run_table_progress.json: Language not supported
  • pkg/service/one2onerestore/testdata/2.run_view_progress.json: Language not supported
  • pkg/service/one2onerestore/testdata/3.run_table_progress.json: Language not supported
  • pkg/service/one2onerestore/testdata/3.run_view_progress.json: Language not supported
  • pkg/service/one2onerestore/testdata/4.run_table_progress.json: Language not supported
  • pkg/service/one2onerestore/testdata/4.run_view_progress.json: Language not supported
  • pkg/service/one2onerestore/testdata/5.run_table_progress.json: Language not supported
  • pkg/service/one2onerestore/testdata/5.run_view_progress.json: Language not supported
  • pkg/service/one2onerestore/testdata/6.run_table_progress.json: Language not supported
  • pkg/service/one2onerestore/testdata/6.run_view_progress.json: Language not supported
Comments suppressed due to low confidence (3)

pkg/service/one2onerestore/progress.go:296

  • The mergeProgressStatus function returns 'in_progress' when merging differing statuses (except if one is 'failed'). Please verify that this behavior is intended for cases where one progress might already be 'done'.
func mergeProgressStatus(dst, src ProgressStatus) ProgressStatus {

pkg/service/one2onerestore/model.go:75

  • [nitpick] Consider renaming 'scyllaTableWithSize' to a more descriptive name such as 'tableToRestore' to clarify its role in tracking table sizes for restoration.
tablesToRestore []scyllaTableWithSize

pkg/service/one2onerestore/progress.go:9

  • [nitpick] Consider renaming the alias 'stderr' for the standard errors package to something more descriptive like 'stdErrors' to avoid potential confusion.
stderr "errors"

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