Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Expose the capability to multiple topics #211

Merged
merged 1 commit into from
Jun 14, 2024
Merged

Conversation

cnweaver
Copy link
Contributor

@cnweaver cnweaver commented Jun 12, 2024

Description

Based on astronomy-commons/adc-streaming#69 , this exposes the same functionality through the hop API.

Checklist

  • All new functions and classes are documented and adhere to Google doc style (3.8.3-3.8.6 of this document)
  • Add/update sphinx documentation with any relevant changes.
  • Add/update pytest-style tests in /tests, ensuring sufficient code coverage.
  • make test runs without errors.
  • make lint doesn't give any warnings.
  • make format doesn't give any code formatting suggestions.
  • make doc runs without errors and generated docs render correctly.
  • Check that CI pipeline run on this PR passes all stages.
  • Review signoff by at least one developer.

@cnweaver cnweaver added the enhancement New feature or request label Jun 12, 2024
@cnweaver cnweaver self-assigned this Jun 12, 2024
Copy link

@jnation3406 jnation3406 left a comment

Choose a reason for hiding this comment

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

Looks fine to me

if len(topics) != 1:
raise ValueError("must specify exactly one topic in write mode")
if topics is None or len(topics) != 1:
topics = [None]

Choose a reason for hiding this comment

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

Is the idea here that if they set multiple topics, you set the topic to None in the Producer so they are forced to manually enter topic in subsequent write calls so there is no ambiguity?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Exactly; the choice we made at the adc level was to store only one topic (https://github.com/astronomy-commons/adc-streaming/blob/b1a5a81c52d2752a9fd63b9717c121285646d09b/adc/producer.py#L89), as either there is a specific topic to which the user wants to always publish or they will have to specify it with ever write, and there didn't seem to be much point in artificially restricting the caller in that latter case to one of the predeclared topics, so we just forget what topics they specified except that there was not exactly one.

@cnweaver cnweaver merged commit 4574bbf into master Jun 14, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants