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.
736a7fd to
54bc50b
Compare
74a4aa8 to
841d1a3
Compare
Query plan asserted in c6c9f04 |
c6c9f04 to
6c0256e
Compare
ReinierMaas
left a comment
There was a problem hiding this comment.
I have a few code simplifications and one API generalisation.
Document InternalProducerClientError raised
Support empty strategic metadata
Add configurable limit to submissions lookup
assert query plan
rm LookupIdsWithEmptyStrategicMetadataError
Call lookup_submission_ids_by_strategic_metadata every test iteration
Simplify query logic
6c0256e to
d276b40
Compare
d276b40 to
14a6a78
Compare
ReinierMaas
left a comment
There was a problem hiding this comment.
I have some questions about the query we build-up. I don't think I fully follow without the recursive example working. And I expect that I hit on a small simplification.
a8bcf9d to
76d4ede
Compare
ReinierMaas
left a comment
There was a problem hiding this comment.
LGTM! I was still thinking about a last small improvement to simplify the query generation code but I am not married to that.
|
The linter still failed on CI: $ git diff
diff --git a/libs/opsqueue_python/tests/test_roundtrip.py b/libs/opsqueue_python/tests/test_roundtrip.py
index 2251c32..1f6ac50 100644
--- a/libs/opsqueue_python/tests/test_roundtrip.py
+++ b/libs/opsqueue_python/tests/test_roundtrip.py
@@ -582,13 +582,19 @@ def test_lookup_too_many_submission_ids_by_strategic_metadata() -> None:
for _ in range(max_ + 1):
assert (
inserted
- == producer_client.lookup_submission_ids_by_strategic_metadata(strategic_metadata)
+ == producer_client.lookup_submission_ids_by_strategic_metadata(
+ strategic_metadata
+ )
)
inserted.append(
- producer_client.insert_submission([1], chunk_size=1, strategic_metadata=strategic_metadata)
+ producer_client.insert_submission(
+ [1], chunk_size=1, strategic_metadata=strategic_metadata
+ )
)
with pytest.raises(TooManyMatchingSubmissionsError) as exc:
assert len(inserted) == max_ + 1
- producer_client.lookup_submission_ids_by_strategic_metadata(strategic_metadata)
+ producer_client.lookup_submission_ids_by_strategic_metadata(
+ strategic_metadata
+ )
assert exc.type is TooManyMatchingSubmissionsError
assert exc.value.max_submissions == max_
|
bump version to 0.36.0
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.