Overridable revision fetching #124
Open
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
There is no need to try and figure out if a revision has already been deployed if we are going to overwrite it anyway. Let's save ourselves the network request!
There are cases where we might want to page through the list of objects available on S3 but we don't need to keep going once we've found what we are looking for. This commit updates `listAllObjects` to accept an `until` argument - if this is provided then when a new page of results is loaded we check if `some` of them match the `until` - if so we don't bother loading another page. A concrete use-case for this is in ember-cli-deploy#120 - sometimes we just want to find the currently active revision so we only need to loop through the "pages" on S3 until we do.
…config So that they can be overridden by call sites who are looking to optimise performance (listing revisions from S3 can be expensive - especially so when you have many deployments deployed)
|
I like it. Let's see if it solves your problem well, and then we can move ahead with this if so. |
|
We've been running this branch for a while now and have seen the following:
With the following config: {
fetchInitialRevisionsFunc(/* context */) {
return () => ({ initialRevisions: [] });
},
fetchRevisionsFunc(/* context */) {
return () => ({ revisions: [] });
},
}We already stored a separate audit log entry in s3 which we are able to grab in order to solve the "what was the previously activated revision" use case. We apply the config above based on an environment variable and only set that variable when calling Given the results above I'll try to add some tests to this branch and aim for it to be mergeable? |
|
Sounds good. Thanks for the update @vitch, and glad it is performing well for you! |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What Changed & Why
Based on conversation in #121, this PR introduces an alternative approach to allowing consumers to optimise list operations on S3.
I'm currently integrating it in to our monorepo but thought I'd chuck this up for feedback first...
Related issues
#121
The changes from #123 are also shown on this branch but should be reviewed separately over there (not sure how to point a GH PR at a branch on a fork without it trying to merge into that fork?)
PR Checklist
People
All maintainers