Lookup submision IDs by strategic metadata#112
Conversation
3b0ca52 to
6f9e1e3
Compare
|
Open question shouldn't an empty |
It depends on the API contract we want to provide. An argument in favour of throwing an error is that if a caller is using an API to "search for submissions matching some strategic metadata" but they haven't provided any strategic metadata, then they possibly have an issue in their code that they should be alerted to. Also, the SQLite query will need a special case to handle this, more code for something we don't have evidence for that it is a usecase we want to support. |
ReinierMaas
left a comment
There was a problem hiding this comment.
I have a few comment and would like to point out that for the other queries we also assert the query plan. This allows us to spot potentially very bad execution behaviour.
| def lookup_submission_ids_by_strategic_metadata( | ||
| self, strategic_metadata: dict[str, int] | ||
| ) -> list[SubmissionId]: |
There was a problem hiding this comment.
I checked the other APIs on the producer and they either provide a single element or an iterator so that it can be lazily evaluated and doesn't need to be materialized in memory all at once.
There was a problem hiding this comment.
The other ProducerClient APIs which are returning iterators are returning chunk iterators. These chunks are deterministically-addressable. Internally, the iterator is keeping track of a submission prefix, a current index, and a max index, then on each call to next() the iterator is performing a network request (GET <object-storage-path><prefix>/<current-index>-out.bin) to fetch the chunk.
This new API is different (not chunks, not querying object storage) from the existing APIs that return iterators, so would need some new wiring put in place to support streaming. But since we are only dealing with submission IDs, it's less of a memory concern. A pragmatic solution for now might be to just implement the configurable upper bound + add some metrics and keep an eye on it.
| async fn lookup_submission_ids_by_strategic_metadata( | ||
| State(state): State<ServerState>, | ||
| extract::Json(strategic_metadata): extract::Json<StrategicMetadataMap>, | ||
| ) -> Result<Json<Vec<SubmissionId>>, ServerError> { |
There was a problem hiding this comment.
Our SubmissionIds are close to u64::max in serialized size, which is reasonable for our random (snowflake) identifiers. This issues the following serialized to JSON response size:
- 1 (
[) + elements * 20 + elements (,|]) characters
For ~1 million this is ~20MB. We can put that as an configurable upper bound to return to the client with an explicit error if it goes over that so that people can update the settings and return the larger response knowingly.
There was a problem hiding this comment.
It might be a bit over-engineered with a newtype that ensures we don't overflow when adding (+1) when building the SQLite query, but.. types good right ? 😅
8057405 to
736a7fd
Compare
Document InternalProducerClientError raised
Support empty strategic metadata
Add configurable limit to submissions lookup
736a7fd to
54bc50b
Compare
This PR adds the
lookup_submission_ids_by_strategic_metadatamethod toProducerClient, which will return the submission IDs of in-progress subsmissions that match ALL of the provided strategic metadata key-value pairs.