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

refactor(general): allow disabling writes on index loads #3645

Merged

Conversation

julio-lopez
Copy link
Collaborator

@julio-lopez julio-lopez commented Feb 13, 2024

This is only a temporary mechanism for changing the default behavior.

Future PRs will:

  • change default value
  • remove index writes altogether from the write path

Followup to: #3603

Ref:

Copy link

codecov bot commented Feb 13, 2024

Codecov Report

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

Comparison is base (cb455c6) 75.86% compared to head (f903815) 77.15%.
Report is 14 commits behind head on master.

Files Patch % Lines
internal/epoch/epoch_manager.go 93.33% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3645      +/-   ##
==========================================
+ Coverage   75.86%   77.15%   +1.29%     
==========================================
  Files         470      470              
  Lines       37301    28446    -8855     
==========================================
- Hits        28299    21948    -6351     
+ Misses       7071     4570    -2501     
+ Partials     1931     1928       -3     

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

@julio-lopez
Copy link
Collaborator Author

@ashmrtn FYI

@@ -1015,16 +1019,28 @@ func rangeCheckpointBlobPrefix(epoch1, epoch2 int) blob.ID {
return blob.ID(fmt.Sprintf("%v%v_%v_", RangeCheckpointIndexBlobPrefix, epoch1, epoch2))
}

func allowWritesOnIndexLoad() bool {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Note: this env variable mechanism to control the behavior is temporary.
We will remove it once the corresponding code paths are explicitly hooked in maintenance.

Copy link
Contributor

@redgoat650 redgoat650 left a comment

Choose a reason for hiding this comment

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

LGTM

@julio-lopez julio-lopez changed the base branch from draft/no-index-cleanup-on-load to master February 16, 2024 18:29
Conditionally disables epoch index maintenace operations when loading indexes.
This prevents (potentially expensive) cleanup write operations on the index
read path.
The behavior is controlled via the `epoch.Manager.allowCleanupWritesOnIndexLoad`
field.

Refs:

- kopia#3174
- kopia#3224
- kopia#3225
@julio-lopez julio-lopez force-pushed the fix/no-index-cleanup-on-load-env-var branch from 1eabe17 to f903815 Compare February 16, 2024 22:31
@julio-lopez julio-lopez merged commit 06ff37f into kopia:master Feb 16, 2024
27 checks passed
@julio-lopez julio-lopez deleted the fix/no-index-cleanup-on-load-env-var branch February 16, 2024 22:59
@Shrekster Shrekster changed the title refactor(general): allow disabling writes on index index loads refactor(general): allow disabling writes on index loads Feb 19, 2024
julio-lopez added a commit to kastenhq/kopia that referenced this pull request Mar 6, 2024
julio-lopez added a commit to kastenhq/kopia that referenced this pull request Mar 7, 2024
julio-lopez added a commit that referenced this pull request Mar 7, 2024
Add
- TestMabyeAdvanceEpoch
- TestMabyeAdvanceEpoch_Empty
- TestMaybeAdvanceEpoch_Error
- TestMaybeAdvanceEpoch_GetParametersError

Ref:
- #3638
- #3645
- #3651
julio-lopez added a commit to kastenhq/kopia that referenced this pull request Mar 12, 2024
julio-lopez added a commit that referenced this pull request Mar 12, 2024
Extracted from #3651.

Thanks to @plar and @redgoat650 for the suggestions.

Ref:

- #3603
- #3645
- #3638
- #3639
Lyndon-Li pushed a commit to Lyndon-Li/kopia that referenced this pull request Mar 18, 2024
…#3645)

Conditionally disables epoch index maintenance operations when loading indexes.
This prevents (potentially expensive) cleanup write operations on the index
read path.
The behavior is controlled via the `epoch.Manager.allowCleanupWritesOnIndexLoad`
field, which can be temporarily overridden via an environment variable.
This override mechanism will be removed in the near future.

Refs:

- kopia#3174
- kopia#3224
- kopia#3225
- kopia#3638
- kopia#3639
Lyndon-Li pushed a commit to Lyndon-Li/kopia that referenced this pull request Mar 22, 2024
…#3645)

Conditionally disables epoch index maintenance operations when loading indexes.
This prevents (potentially expensive) cleanup write operations on the index
read path.
The behavior is controlled via the `epoch.Manager.allowCleanupWritesOnIndexLoad`
field, which can be temporarily overridden via an environment variable.
This override mechanism will be removed in the near future.

Refs:

- kopia#3174
- kopia#3224
- kopia#3225
- kopia#3638
- kopia#3639
julio-lopez added a commit that referenced this pull request Mar 27, 2024
…3651)

Perform index epoch compaction and cleanup during repository maintenance

* refactor: rename maintenance task for deleting superseded indexes
* maintenance task to cleanup epoch markers
* maintenance task to advance write index epoch
* maintenance task to compact epoch ranges
* quick maintenance task to compact a single (index) epoch
* full maintenance task to compact a single (index) epoch

Ref:
- #3638
- #3639

Followup to:
- #3603
- #3645

Related helpers and changes:
- #3721
- #3735
- #3709
- #3728
- #3727
- #3726
Lyndon-Li pushed a commit to Lyndon-Li/kopia that referenced this pull request Mar 29, 2024
…opia#3651)

Perform index epoch compaction and cleanup during repository maintenance

* refactor: rename maintenance task for deleting superseded indexes
* maintenance task to cleanup epoch markers
* maintenance task to advance write index epoch
* maintenance task to compact epoch ranges
* quick maintenance task to compact a single (index) epoch
* full maintenance task to compact a single (index) epoch

Ref:
- kopia#3638
- kopia#3639

Followup to:
- kopia#3603
- kopia#3645

Related helpers and changes:
- kopia#3721
- kopia#3735
- kopia#3709
- kopia#3728
- kopia#3727
- kopia#3726
julio-lopez added a commit that referenced this pull request Apr 1, 2024
Change default 'allowWritesOnIndexLoad' to false when env var is unset
Add TestNoEpochAdvanceOnIndexRead

Ref:
- Followup to #3645
- Avoid index (epoch) cleanup and compaction during index reads #3638
- Make "read" commands/operations really read-only.  #3639
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.

None yet

2 participants