Support Query input from URL on report collector side + integration test #1510
Conversation
| Ok(manifest_path) | ||
| } | ||
|
|
||
| fn create_upload_file<const SHARDS: usize>( |
There was a problem hiding this comment.
this works, but my original plan was to generate SHARDS raw input files, then encrypt each with --length-delimited. either way is fine with me, this just puts a little extra weight on the rust implementation.
There was a problem hiding this comment.
how would you run ipa in the clear on those?
| let enc2 = dir.path().join("helper2.enc"); | ||
| let enc3 = dir.path().join("helper3.enc"); | ||
|
|
||
| let poll_port = TcpListener::bind("127.0.0.1:0").unwrap(); |
There was a problem hiding this comment.
should this be some random high value port?
There was a problem hiding this comment.
That's what :0 will do.
ipa-core/src/bin/report_collector.rs
Outdated
| panic!("Either --url-file-list or --enc-input-file1, --enc-input-file2, and --enc-input-file3 must be provided"); | ||
| } | ||
| }, | ||
| // encrypted_inputs, |
There was a problem hiding this comment.
| // encrypted_inputs, |
|
I didn't have a chance to review #1508 yet, but the integration test changes look good to me. |
ipa-core/src/bin/report_collector.rs
Outdated
| } | ||
| if helper_input.len() != shard_count { | ||
| return Err(format!( | ||
| "Helper {helper_id} does not have enough input. Expected {shard_count}, got {}", |
There was a problem hiding this comment.
This is a nit given the nature of this exercise, but: maybe just read all the lines immediately and complain if the file doesn't have the expected number of lines (3 * N)?
Attributing the incorrect length to a particular helper may be confusing, and this will accept a file that is too long, including e.g. if the length is 3 * M for an incorrect number of shards M > N.
| let enc2 = dir.path().join("helper2.enc"); | ||
| let enc3 = dir.path().join("helper3.enc"); | ||
|
|
||
| let poll_port = TcpListener::bind("127.0.0.1:0").unwrap(); |
There was a problem hiding this comment.
That's what :0 will do.
ipa-core/tests/hybrid.rs
Outdated
|
|
||
| // update manifest file | ||
| for (path, mut file) in files { | ||
| // let path = path.strip_prefix(dest_dir).unwrap().to_str().unwrap(); |
There was a problem hiding this comment.
| // let path = path.strip_prefix(dest_dir).unwrap().to_str().unwrap(); |
ipa-core/tests/hybrid.rs
Outdated
| ]) | ||
| .silent(); | ||
|
|
||
| let _server_handle = command.spawn().unwrap(); |
There was a problem hiding this comment.
| let _server_handle = command.spawn().unwrap(); | |
| let _server_handle = command.spawn().unwrap().terminate_on_drop(); |
akoshelev
force-pushed
the
rc-stream-upload
branch
from
e1b8ffc to
329328c
Compare
|
#1514 will fix the test failures. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1510 +/- ##
==========================================
- Coverage 93.20% 93.05% -0.16%
==========================================
Files 241 241
Lines 44054 44126 +72
==========================================
- Hits 41061 41060 -1
- Misses 2993 3066 +73 ☔ View full report in Codecov by Sentry. |
|
yea we don't get coverage for integration tests :( |
This builds on top #1509 and #1508 and supports polling for inputs mode on the report collector side and adds an integration test to verify correctness of this setup end-to-end.
Test passes, so it seems we are quite close