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

feat: idsse-897 rabbitmq_utils Rpc #87

Merged
merged 31 commits into from
Dec 6, 2024

Conversation

mackenzie-grimes-noaa
Copy link
Contributor

@mackenzie-grimes-noaa mackenzie-grimes-noaa commented Nov 22, 2024

NOTE: depends on PR #88 <-- merged

Linear Issue

IDSSE-897

Changes

  • Create Rpc class in rabbitmq_utils
    • Creates threadsafe Consumer
    • Uses Publisher logic to blocking_publish messages to an exchange, but reuses the Consumer's thread which is a constraint of Direct Reply-to
    • Consumer then listens to responses coming back over RabbitMQ's built-in Direct Reply-to queue and returns the response message to the caller when it is received

Callers can simple instantiate an Rpc class (providing the params of the exchange to publish to), call start(), then publish messages with send_request(). This method will block until the receiving RMQ service sends a response (or times out), formatted as an RpcResponse (containing the body and properties of the message).

Explanation

This type of logic existed in Risk Processor (called "RpcClient"), but moving it here and correctly using these newer rabbitmq_utils classes helps standardize behavior across IDSSe services.

@mackenzie-grimes-noaa
Copy link
Contributor Author

mackenzie-grimes-noaa commented Nov 22, 2024

This is not ready, but I don't want to lose it over the Thanksgiving break. Unit tests are failing, and I need to finish unit tests for the new Rpc class.

Update: unit tests exist now.

@mackenzie-grimes-noaa mackenzie-grimes-noaa marked this pull request as ready for review December 4, 2024 21:15
Geary-Layne
Geary-Layne previously approved these changes Dec 5, 2024
Copy link
Contributor

@Geary-Layne Geary-Layne left a comment

Choose a reason for hiding this comment

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

Looks good. Would you please create a task to make sure we have good test coverage for Publisher/Consumer.

python/idsse_common/idsse/common/rabbitmq_utils.py Outdated Show resolved Hide resolved
@@ -98,6 +98,12 @@ class PublishMessageParams(NamedTuple):
route_key: str | None = None


class RpcResponse(NamedTuple):
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this class in addition to PublishMessageParams?

Copy link
Contributor Author

@mackenzie-grimes-noaa mackenzie-grimes-noaa Dec 6, 2024

Choose a reason for hiding this comment

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

Technically no, but PublishMessageParams is used for outbound RMQ messages and RpcResponse was originally written for inbound responses. I just don't know what to name the class if it's used for both.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe "RabbitMqMessage"?

Copy link
Contributor

Choose a reason for hiding this comment

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

That works, or RmqMessage if you want shorter.

@@ -117,7 +117,6 @@ def exec_cmd(commands: Sequence[str], timeout: int | None = None) -> Sequence[st

def to_iso(date_time: datetime) -> str:
"""Format a datetime instance to an ISO string"""
logger.debug('Datetime (%s) to iso', datetime)
Copy link
Contributor

Choose a reason for hiding this comment

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

I've wondered if these need to be here a few times.

RpcResponse | None: The response message (body and properties), or None on request
timeout or error handling response.
"""
if not self.is_open:
Copy link
Contributor

Choose a reason for hiding this comment

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

I like that this gets handled

@mackenzie-grimes-noaa mackenzie-grimes-noaa merged commit 46fce65 into main Dec 6, 2024
2 checks passed
@mackenzie-grimes-noaa mackenzie-grimes-noaa deleted the feat/idsse-897/rabbitmq-rpc branch December 6, 2024 20:26
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.

2 participants