Skip to content

Lookup submision IDs by strategic metadata#112

Open
jerbaroo wants to merge 16 commits into
masterfrom
jerbaroo-64-lookup-by-strategic-metadata
Open

Lookup submision IDs by strategic metadata#112
jerbaroo wants to merge 16 commits into
masterfrom
jerbaroo-64-lookup-by-strategic-metadata

Conversation

@jerbaroo

@jerbaroo jerbaroo commented Jun 8, 2026

Copy link
Copy Markdown
Contributor

This PR adds the lookup_submission_ids_by_strategic_metadata method to ProducerClient, which will return the submission IDs of in-progress subsmissions that match ALL of the provided strategic metadata key-value pairs.

    def lookup_submission_ids_by_strategic_metadata(
        self, strategic_metadata: dict[str, int]
    ) -> list[SubmissionId]:
        """Attempts to find in-progress submissions where the strategic metadata
        of that submission includes all of the key-value pairs of the given
        'strategic_metadata'. A matching submission must include all of the
        given key-value pairs, but it may also contain other key-value pairs.

        Raises:
        - `TooManyMatchingSubmissionsError` if the lookup matches more
          submissions than the server's configured maximum. Narrow the query
          with more specific strategic metadata.
        - `InternalProducerClientError` if there is a low-level internal error.

        """

@jerbaroo jerbaroo requested a review from Qqwy June 8, 2026 15:58
@jerbaroo jerbaroo force-pushed the jerbaroo-64-lookup-by-strategic-metadata branch from 3b0ca52 to 6f9e1e3 Compare June 8, 2026 15:58
@jerbaroo jerbaroo requested a review from ReinierMaas June 23, 2026 12:30
@ReinierMaas

Copy link
Copy Markdown
Contributor

Open question shouldn't an empty StrategicMetadataMap return all submissions instead of raising an error on that?
Also this is an easy way to DDoS OpsQueue if the number of returned submissions is large.

@jerbaroo

Copy link
Copy Markdown
Contributor Author

Open question shouldn't an empty StrategicMetadataMap return all submissions instead of raising an error on that?

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 ReinierMaas left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread libs/opsqueue_python/python/opsqueue/producer.py Outdated
Comment thread libs/opsqueue_python/python/opsqueue/producer.py
Comment thread opsqueue/src/producer/server.rs Outdated
Comment thread libs/opsqueue_python/python/opsqueue/producer.py Outdated
@jerbaroo jerbaroo force-pushed the jerbaroo-64-lookup-by-strategic-metadata branch 4 times, most recently from 736a7fd to 54bc50b Compare July 7, 2026 15:48
@jerbaroo jerbaroo force-pushed the jerbaroo-64-lookup-by-strategic-metadata branch from 74a4aa8 to 841d1a3 Compare July 9, 2026 14:26
@jerbaroo

jerbaroo commented Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

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.

Query plan asserted in c6c9f04

@jerbaroo jerbaroo requested a review from ReinierMaas July 9, 2026 14:27
@jerbaroo jerbaroo force-pushed the jerbaroo-64-lookup-by-strategic-metadata branch 2 times, most recently from c6c9f04 to 6c0256e Compare July 9, 2026 16:31

@ReinierMaas ReinierMaas left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have a few code simplifications and one API generalisation.

Comment thread justfile
Comment thread libs/opsqueue_python/src/errors.rs
Comment thread libs/opsqueue_python/python/opsqueue/producer.py Outdated
Comment thread opsqueue/src/common/mod.rs Outdated
Comment thread libs/opsqueue_python/tests/test_roundtrip.py Outdated
Comment thread opsqueue/src/common/submission.rs
Comment thread opsqueue/src/common/submission.rs Outdated
Comment thread opsqueue/src/common/submission.rs Outdated
Comment thread opsqueue/src/common/submission.rs Outdated
@jerbaroo jerbaroo force-pushed the jerbaroo-64-lookup-by-strategic-metadata branch from d276b40 to 14a6a78 Compare July 14, 2026 13:32
@jerbaroo jerbaroo requested a review from ReinierMaas July 14, 2026 13:32

@ReinierMaas ReinierMaas left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread libs/opsqueue_python/tests/test_roundtrip.py Outdated
Comment thread opsqueue/src/common/submission.rs Outdated
Comment thread opsqueue/src/common/submission.rs Outdated
Comment thread opsqueue/src/common/submission.rs Outdated
Comment thread opsqueue/src/common/submission.rs Outdated
Comment thread opsqueue/src/common/submission.rs Outdated
@jerbaroo jerbaroo force-pushed the jerbaroo-64-lookup-by-strategic-metadata branch from a8bcf9d to 76d4ede Compare July 15, 2026 08:21
@jerbaroo jerbaroo requested a review from ReinierMaas July 15, 2026 08:22

@ReinierMaas ReinierMaas left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! I was still thinking about a last small improvement to simplify the query generation code but I am not married to that.

Comment thread opsqueue/src/common/submission.rs
Comment thread opsqueue/src/common/submission.rs
@ReinierMaas

Copy link
Copy Markdown
Contributor

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_

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Support checking whether a submission currently exists in the queue for a particular piece of strategic metadata

2 participants