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

BREAKING: feat: get queued outbound message in transport handle message #2170

Conversation

dbluhm
Copy link
Contributor

@dbluhm dbluhm commented Mar 15, 2023

This will enable queues to annotate outbound messages with relevant context for things like dead letter queues that know which wallet sent the message that failed, what message was being sent, etc.

This is a breaking change for plugins adding queue implementations (cc @shaangill025) but we think this is worthwhile for the ability to improve internal error handling.

dbluhm added 2 commits March 3, 2023 09:12
This will enable queues to annotate outbound messages with relevant
context for things like dead letter queues that know which wallet sent
the message that failed.

Signed-off-by: Daniel Bluhm <[email protected]>
@ianco ianco requested a review from shaangill025 March 15, 2023 16:18
@ianco ianco changed the title feat: get queued outbound message in transport handle message BREAKING: feat: get queued outbound message in transport handle message Mar 15, 2023
@swcurran
Copy link
Contributor

@dbluhm -- is this a "hold 0.8.0 release until this is in", or is it OK to wait for the next release?

@dbluhm
Copy link
Contributor Author

dbluhm commented Mar 15, 2023

This can wait (sorry for the slow response and glad you didn't wait on me lol)

@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@dbluhm
Copy link
Contributor Author

dbluhm commented Jun 7, 2023

@swcurran I'd like to see this PR merged but I think the breaking change status has scared us away from merging so far 😅 To reiterate, this breaking change affects only message queue plugins and will not affect deployments not using these plugins. For message queue plugins, the required modifications should be pretty minimal; we have a branch for the Kafka plugin already that incorporates this change. Updating the Redis plugin should be pretty trivial.

@swcurran
Copy link
Contributor

swcurran commented Jul 5, 2023

@dbluhm -- shall we merge this one next? @usingtechnology, @Jsyro, @shaangill025 -- who is the best to coordinate what changes need to be made to the plugins for Redis once this is merged.

@sonarqubecloud
Copy link

sonarqubecloud bot commented Jul 5, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

@usingtechnology
Copy link
Contributor

i guess it will depend on workloads. if @shaangill025 is busy, i could probably jump in and tackle it. do we know if the redis queue plugin is in production anywhere?

@swcurran
Copy link
Contributor

swcurran commented Jul 5, 2023

It is being used in BCGov by at least the Mediator dev/test. It may be in use in the prod instance as well.

@swcurran
Copy link
Contributor

swcurran commented Jul 5, 2023

Please do. We’re pushing on getting a bunch of merges done now that we have 0.8.2 out the door.

@shaangill025
Copy link
Contributor

I think the changes in Redis plugin to work with this PR will involve the following:
https://github.com/bcgov/aries-acapy-plugin-redis-events/blob/a27cefe5de1b8e74e3c154bf9c782b6dcb4b464b/redis_queue/v1_0/outbound.py#L63

async def handle_message(
      self,
      profile: Profile,
      outbound_message: QueuedOutboundMessage,
      endpoint: str,
      metadata: dict = None,
      api_key: str = None,
  ):
      """Prepare and send message to external queue."""
      payload = outbound_message.payload

Also adding QueuedOutboundMessage import here

dbluhm added a commit to dbluhm/aries-acapy-plugin-redis-events that referenced this pull request Jul 5, 2023
This updates to align with changes made upstream in:

openwallet-foundation/acapy#2170

Signed-off-by: Daniel Bluhm <[email protected]>
dbluhm added a commit to dbluhm/aries-acapy-plugin-redis-events that referenced this pull request Jul 5, 2023
This updates to align with changes made upstream in:

openwallet-foundation/acapy#2170

Signed-off-by: Daniel Bluhm <[email protected]>
@dbluhm
Copy link
Contributor Author

dbluhm commented Jul 5, 2023

As @shaangill025 outlined, changes were pretty minimal so I went ahead and opened a PR with those changes over there.

@dbluhm
Copy link
Contributor Author

dbluhm commented Jul 5, 2023

Please do. We’re pushing on getting a bunch of merges done now that we have 0.8.2 out the door.

A little unclear to me: are there external dependencies blocking this PR specifically or can we go ahead and merge?

@swcurran
Copy link
Contributor

swcurran commented Jul 5, 2023

No. We’ll merge. We are also going to do a tag on the Redis plugin on the commit before merging this one to point out the breaking change.

@swcurran swcurran merged commit dd3c864 into openwallet-foundation:main Jul 5, 2023
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.

4 participants