[2/4][aggregator] implement fetch key #422
Open
Conversation
This file contains hidden or 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
benr-ml
reviewed
Jan 8, 2026
| } | ||
|
|
||
| /// Middleware to add aggregator's own version and git version to headers. | ||
| async fn add_response_headers(mut response: Response) -> Response { |
Collaborator
There was a problem hiding this comment.
we already have this function somewhere, no?
| Err(e) => { | ||
| let msg = format!("Aggregating responses failed: {e}"); | ||
| error!("{}", msg); | ||
| Err(ErrorResponse::from(InternalError::Failure(msg))) |
| let response = client | ||
| .post(format!("{}/v1/fetch_key", member.url)) | ||
| .header("Client-Sdk-Type", "aggregator") | ||
| .header("Client-Sdk-Version", package_version!()) |
Collaborator
There was a problem hiding this comment.
let's make all these constants and use them here and in the key server instead of duplicating (possible errors)
| .unwrap_or("unknown") | ||
| ); | ||
|
|
||
| validate_key_server_version(version).map_err(ErrorResponse::from)?; |
Collaborator
There was a problem hiding this comment.
from is not called here implicitly?
| )) | ||
| .and_then(|v| { | ||
| v.to_str() | ||
| .map_err(|_| InternalError::InvalidKeyServerVersion) |
Collaborator
There was a problem hiding this comment.
why not use Failure instead of InvalidKeyServerVersion? this error means that the aggregator cannot perform its job, and the actual reason here does not matter (e.g., is there a difference to the caller between a server that didn't reply vs a server that replied with old version?)
Collaborator
There was a problem hiding this comment.
we should log and monitor those errors differently ofc
joyqvq
force-pushed
the
joy/move-aggregator
branch
2 times, most recently
from
6bf4a3e to
998ed57
Compare
joyqvq
force-pushed
the
joy/impl-fetch-key
branch
2 times, most recently
from
6e54905 to
7e57d6e
Compare
joyqvq
force-pushed
the
joy/handle-aggregator-in-ks
branch
from
f8cd8f9 to
9fb649c
Compare
joyqvq
force-pushed
the
joy/impl-fetch-key
branch
from
7e57d6e to
23900f6
Compare
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.
Description
impl fetch key for aggregator: calls to member urls to get keys for all ids. if one of the keys from server is invalid, discard all keys from this server. if threshold is reached (count only all keys are returned from the server), aggregator and return to client.
version handling: aggregator maintains min_sdk_version. checks client request against it. then calls to each key server with aggregator version. key server checks against min_aggregator_version and return its key server version. aggregator checks key-server-version is at least aggregator's own version. then return client key-server-version set to aggregator version
error handling: aggregator record majority error relayed from key servers and return, everything else as internal error
Test plan
tests here focus on version and error. e2e test for aggregation is in the next pr.