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

release-23.2: restore: limit restore spans to 200 files #119883

Merged
merged 1 commit into from Mar 11, 2024

Conversation

blathers-crl[bot]
Copy link

@blathers-crl blathers-crl bot commented Mar 4, 2024

Backport 1/1 commits from #119840 on behalf of @dt.

/cc @cockroachdb/release


Previously we would limit restore spans to 384MB, but if a backup contained many small files, a large number of these files could be grouped into such a span before it hit the 384MB target. Unfortunately this caused the restore process to open all of those files to process the restore span, leading to very large numbers of concurrent connections when restoring a backup consisting of tiny files.

This patch limits the restore spans to 384MB or 200 files, whichever is hit first. Restores of backups with large numbers of tiny files may thus be slightly slower but will better limit their concurrent outbound connections.

Fixes #119785.

NB: This breaks the test cases for the experimental/disabled support for memory-monitored restores. Given we have no plans to actually enable these at this time, but do need to fix this bug in the normal restore path and backport such a fix, to reduce churn in said backports the tests of the disabled mode are simply skipped in this diff rather modified, in anticipation of removing them completely from the development branch, tracked in #119836.

Release note (bug fix): fix a bug where RESTORE on certain backups would open a very large number of concurrent connections to the backup storage provider.

Epic: none.


Release justification: bug fix.

Previously we would limit restore spans to 384MB, but if a backup contained
many small files, a large number of these files could be grouped into such
a span before it hit the 384MB target. Unfortunately this meant opening a
all of those files to process the restore span, leading to very large numbers
of concurrent connections when restoring a backup consisting of tiny files.

This patch limits the restore spans to 384MB or 200 files, whichever is
hit first. Restores of backups with large numbers of tiny files may thus
be slightly slower but will better limit their concurrent outbound connections.

This breaks the test cases for experimental/disabled support for memory-monitored
restores. Given we have no plans to actually enable this at this time, for
the sake of ease of backporting this bugfix, these tests are simply skipped rather
than introducing churn to them. #119836 tracks removing them and that functionality
entirely instead, but again for the same of backporting is not done here.

Release note (bug fix): fix a bug where RESTORE on certain backups would open a very large number of connections to the backup storage provider.
Epic: none.
@blathers-crl blathers-crl bot requested a review from a team as a code owner March 4, 2024 20:12
@blathers-crl blathers-crl bot requested review from msbutler and removed request for a team March 4, 2024 20:12
@blathers-crl blathers-crl bot added the blathers-backport This is a backport that Blathers created automatically. label Mar 4, 2024
@blathers-crl blathers-crl bot force-pushed the blathers/backport-release-23.2-119840 branch from 8a8474c to 5846bf2 Compare March 4, 2024 20:12
@blathers-crl blathers-crl bot added the O-robot Originated from a bot. label Mar 4, 2024
@blathers-crl blathers-crl bot force-pushed the blathers/backport-release-23.2-119840 branch from be8fc3b to 343e3e8 Compare March 4, 2024 20:12
@blathers-crl blathers-crl bot assigned dt Mar 4, 2024
@blathers-crl blathers-crl bot requested a review from dt March 4, 2024 20:12
Copy link
Author

blathers-crl bot commented Mar 4, 2024

Thanks for opening a backport.

Please check the backport criteria before merging:

  • Backports should only be created for serious
    issues
    or test-only changes.
  • Backports should not break backwards-compatibility.
  • Backports should change as little code as possible.
  • Backports should not change on-disk formats or node communication protocols.
  • Backports should not add new functionality (except as defined
    here).
  • Backports must not add, edit, or otherwise modify cluster versions; or add version gates.
  • All backports must be reviewed by the owning areas TL and one additional
    TL. For more information as to how that review should be conducted, please consult the backport
    policy
    .
If your backport adds new functionality, please ensure that the following additional criteria are satisfied:
  • There is a high priority need for the functionality that cannot wait until the next release and is difficult to address in another way.
  • The new functionality is additive-only and only runs for clusters which have specifically “opted in” to it (e.g. by a cluster setting).
  • New code is protected by a conditional check that is trivial to verify and ensures that it only runs for opt-in clusters. State changes must be further protected such that nodes running old binaries will not be negatively impacted by the new state (with a mixed version test added).
  • The PM and TL on the team that owns the changed code have signed off that the change obeys the above rules.
  • Your backport must be accompanied by a post to the appropriate Slack
    channel (#db-backports-point-releases or #db-backports-XX-X-release) for awareness and discussion.

Also, please add a brief release justification to the body of your PR to justify this
backport.

@blathers-crl blathers-crl bot requested a review from stevendanna March 4, 2024 20:12
@blathers-crl blathers-crl bot added the backport Label PR's that are backports to older release branches label Mar 4, 2024
@cockroach-teamcity
Copy link
Member

This change is Reviewable

@dt dt requested a review from jbowens March 5, 2024 13:45
Copy link
Collaborator

@jbowens jbowens left a comment

Choose a reason for hiding this comment

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

lgtm

@dt dt merged commit b6b08d7 into release-23.2 Mar 11, 2024
5 of 6 checks passed
@dt dt deleted the blathers/backport-release-23.2-119840 branch March 11, 2024 15:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport Label PR's that are backports to older release branches blathers-backport This is a backport that Blathers created automatically. O-robot Originated from a bot.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants