add CLI command to run hybrid query #1472
Conversation
|
not sure what you mean by HTTPS clients - we do have them and they fully support HTTPS |
|
on the integration test side, #1473 should provide a way to write tests that leverage HTTPS |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1472 +/- ##
==========================================
- Coverage 93.84% 93.49% -0.35%
==========================================
Files 235 236 +1
Lines 42453 42627 +174
==========================================
+ Hits 39839 39855 +16
- Misses 2614 2772 +158 ☔ View full report in Codecov by Sentry. |
eriktaubeneck
force-pushed
the
hybrid-rc-cli
branch
from
7651bd0 to
4d19d4e
Compare
| let leader_clients = &clients[0]; | ||
| try_join_all( | ||
| inputs | ||
| .into_iter() | ||
| .zip(leader_clients) | ||
| .map(|(input_stream, client)| { | ||
| client.query_input(QueryInput { | ||
| query_id, | ||
| input_stream, | ||
| }) | ||
| }), | ||
| ) | ||
| .await | ||
| .unwrap(); |
There was a problem hiding this comment.
@akoshelev I ended up needing to create a new function here that could take Vec<[IpaHttpClient<Helper>; 3]>. right now it submits everything to the leader, but this seems like the natural place to do the round robin submission.
| @@ -329,6 +358,94 @@ fn write_ipa_output_file( | |||
| Ok(()) | |||
| } | |||
|
|
|||
| fn write_hybrid_output_file( | |||
There was a problem hiding this comment.
this looks oddly familiar, is it possible to re-use the same code and avoid copy paste here?
There was a problem hiding this comment.
same old problem with Oprf and Hybrid. we can either spend time supporting both or just delete the other when we remove Oprf.
| query_type, | ||
| }; | ||
|
|
||
| let query_id = helper_clients[0][0] |
There was a problem hiding this comment.
should we first do a query status to make sure something else isn't running?
There was a problem hiding this comment.
I believe it panics with "query already running" already, so that's effectively the same, no?
There was a problem hiding this comment.
Not really. The system does't have something like distributed transactions. Think of the following scenario; helper 1 gets the create_query and sends prepare_query to 2 and 3. Say helper 2 return ok but helper 3 says its running something return error. We don't "rollback" helper2 so it's left awaiting inputs.
This problem should be worse now with more states spread around.
There was a problem hiding this comment.
That's fair, and we have the "kill" endpoint to help clean up that state that sometimes happens. I'm inclined to try and get this merged as is (which is the same as Oprf Ipa, but with shards), and punt on making this better once we can actually run a query. For the immediate testing needs, we'll be restarting the binaries regularly anyways.
There was a problem hiding this comment.
Don't think we have a sharded kill, but we can easily restart the k8s cluster.
I you think adding a query_status is going to take much time then I agree.
There was a problem hiding this comment.
certainly something we should add, but we likely want to do that across all shards, so let's punt for now.
|
only check fail is code patch, which is coming in #1476. merging |
eriktaubeneck
merged commit 26ee498
into
private-attribution:main
turns out a bunch of these functions aren't actually OprfIpa specific. I did one small "hack" to convert
HybridQueryParamsintoIpaQueryConfig(it's just a super set and the extra fields are unused at this point) to avoid copying and pasting the entirerun_query_and_validatefunction.This is mostly complete, but we'll need to swap in
HTTPSclients instead ofHTTP, so we're blocked on that, cc @akoshelev