feat: Don't upload chunks that are already on the server #1651
Conversation
There was a problem hiding this comment.
Nice work, left a comment.
I have already started the PR on the server side but as of now I don't expect to finish it by today, might be able by the end of the week. We should also test very well the new behavior before releasing the CLI, what is the best way to do it? Just pulling your branch and building it locally?
src/utils/file_upload.rs
Outdated
| api.assemble_release_artifacts(context.org, context.release()?, checksum, &checksums)? | ||
| }; | ||
|
|
||
| response.missing_chunks |
There was a problem hiding this comment.
In addition, just to avoid regressions, I would return the missing chunks only for artifact bundle since we don't want to support that for the old endpoint. Technically it shouldn't cause any problems since that endpoint always returns an empty array but I would maybe guard the code against this behavior.
|
Blocked by #1652. |
loewenheim
force-pushed
the
feat/dedupe-artifacts
branch
from
14c4e1b
to
f0bab45
Compare
|
Tests are now fixed. I added |
| // Filter out chunks that are already on the server. This only matters if we're in the | ||
| // `assemble_artifact_bundle` case; `assemble_release_artifacts` always returns `missing_chunks: []`, | ||
| // so there's no point querying the endpoint. | ||
| if options.supports(ChunkUploadCapability::ArtifactBundles) && context.project.is_some() { |
There was a problem hiding this comment.
Probably not worth it, but we might want to extract this (and another use above) into use_artifact_bundle or similar helper, to make sure it's always in sync in all places. But it's only repeated twice right now, so no need to move it yet imo.
| @@ -21,7 +21,7 @@ fn command_bundle_jvm_out_not_found_creates_dir() { | |||
| testcase_cwd_path.join("jvm"), | |||
| ) | |||
| .unwrap(); | |||
| let _upload_endpoints = mock_common_upload_endpoints(ServerBehavior::Legacy); | |||
| let _upload_endpoints = mock_common_upload_endpoints(ServerBehavior::Legacy, vec![], None); | |||
There was a problem hiding this comment.
It's fine for now, but keep in mind to maybe one day refactor it into an options struct, otherwise this args list can grooow and groow
There was a problem hiding this comment.
I refactored it, I think you're right that it's nicer with an options struct.
…)" This reverts commit 7470bac.
Currently `upload_files_chunked works like this:
assemble_artifact_bundle/assemble_release_artifactsin a loop until it reports a success.This restructures
upload_files_chunkedso instead it works like this:assemble_artifact_bundle/assemble_release_artifactsonce and get back a list of missing chunks.Unfortunately, I don't see a good way to test this; the mocked endpoint needs to return some list of missing chunks, but the chunks are different every time you run the test. Therefore, AFAICT, the only sensible value to return is
[]. This means that nothing will be "uploaded" (because no chunks are reported as missing) and the upload endpoint never gets aPOSTrequest.