Skip to content

Extract get used blobs in prune as separate function#2841

Merged
MichaelEischer merged 1 commit intorestic:masterfrom
aawsome:optimize-getUsedBlobs
Aug 1, 2020
Merged

Extract get used blobs in prune as separate function#2841
MichaelEischer merged 1 commit intorestic:masterfrom
aawsome:optimize-getUsedBlobs

Conversation

@aawsome
Copy link
Copy Markdown
Contributor

@aawsome aawsome commented Jul 19, 2020

What is the purpose of this change? What does it change?

Put getting used blob within prune into a separate function

Was the change discussed in an issue or in the forum before?

This is a part of #2718 that can be merged independently.

Checklist

  • I have read the Contribution Guidelines
  • I have enabled maintainer edits for this PR
  • I have not added tests for all changes in this PR - tests cover this change
  • I have not added documentation for the changes (in the manual)
  • There's a new file in changelog/unreleased/ that describes the changes for our users (template here) - is this needed?
  • I have run gofmt on the code in all commits
  • All commit messages are formatted in the same style as the other commits in the repo
  • I'm done, this Pull Request is ready for review

@aawsome aawsome mentioned this pull request Jul 19, 2020
11 tasks
@aawsome aawsome force-pushed the optimize-getUsedBlobs branch from e482ec9 to e1018d9 Compare July 19, 2020 09:15
@MichaelEischer
Copy link
Copy Markdown
Member

This PR also contains the code of #2840, please remove it. With #2599 merged, this just extracts getUsedBlobs into a separate function?

@aawsome
Copy link
Copy Markdown
Contributor Author

aawsome commented Aug 1, 2020

This PR also contains the code of #2840, please remove it.

This is not intended; however I rebased this to an old version of #2840; so yes, it is still included. However, I can change this.

With #2599 merged, this just extracts getUsedBlobs into a separate function?

After #2599 is merged this in fact is no longer an optimization. I just extracted the functionality "get used blobs of all given snapshots and print a progress bar). The reason is that #2718 still uses this functionality, however directly at the beginning. Extracting it into a function IMO makes the new prune implementation easier to read.

@aawsome aawsome force-pushed the optimize-getUsedBlobs branch from e1018d9 to 9762bec Compare August 1, 2020 19:08
@aawsome aawsome changed the title Optimize get used blobs extract get used blobs in prune as separate function Aug 1, 2020
@aawsome aawsome changed the title extract get used blobs in prune as separate function Extract get used blobs in prune as separate function Aug 1, 2020
Copy link
Copy Markdown
Member

@MichaelEischer MichaelEischer left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for the cleanup.

Extracting it into a function IMO makes the new prune implementation easier to read.

Not just your new implementation. pruneRepository is already a way too long function, so extracting small independent parts helps.

@MichaelEischer
Copy link
Copy Markdown
Member

Directly looking at travis shows that the CI run was completed successfully. So I'll just ignore that it still shows as 'in progress'.

@MichaelEischer MichaelEischer merged commit 66d089e into restic:master Aug 1, 2020
@aawsome aawsome deleted the optimize-getUsedBlobs branch August 2, 2020 04:35
@aawsome aawsome mentioned this pull request Aug 3, 2020
8 tasks
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