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

Refactor outbound queue interface #1348

Merged

Conversation

dbluhm
Copy link
Contributor

@dbluhm dbluhm commented Aug 11, 2021

This PR adjusts the outbound queue interface to be more easily generalized. As we've worked to implement outbound queuing via Kafka, we've found that the configuration just did not map onto what was needed in Kafka. Rather than expecting a connection string and parsing protocol etc. etc., this PR just passes all settings to the outbound queue implementation and allows it to read plugin configuration from the settings. This affords much greater flexibility in the structure of the outbound queue implementation. As a result, I removed some outbound queue command line arguments that were no longer used.

Additionally, I updated aioredis to 2.0.0. From the aioredis maintainers speaking on version 1.3.1 of the library:

As of December, 2019, this library had been abandoned. The code had grown stale, issues were piling up, and we were rapidly becoming out-of-date with Redis’s own feature-set. In light of this, the decision was made to ensure that the barrier to support for potential maintainers or contributors would be minimized.

I found that using aioredis 2.0.0 significantly simplified the code as it stood.

Some other minor consistency fixes were made to bring the outbound queue package in line with the rest of the code base.

I haven't had an opportunity to integration test the redis changes just yet. I noticed there were some tests for this but they did not appear to be run anywhere in the tests run by the github actions. Is this something we would want run as part of PR checks or is the occasional one-off sufficient?

@dbluhm
Copy link
Contributor Author

dbluhm commented Aug 11, 2021

Alternatively, I wonder if the redis implementation should live a little bit further distant from the ACA-Py core. Have we considered including a separate package in the repo for these sorts of plugins?

@codecov-commenter
Copy link

codecov-commenter commented Aug 11, 2021

Codecov Report

Merging #1348 (daba217) into main (9541edf) will decrease coverage by 0.01%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             main    #1348      +/-   ##
==========================================
- Coverage   95.43%   95.41%   -0.02%     
==========================================
  Files         476      476              
  Lines       28988    28962      -26     
==========================================
- Hits        27664    27634      -30     
- Misses       1324     1328       +4     

burdettadam
burdettadam previously approved these changes Aug 11, 2021
dbluhm added 2 commits August 11, 2021 15:45
This value is not used but making this fix anyways for consistency

Signed-off-by: Daniel Bluhm <[email protected]>
@andrewwhitehead andrewwhitehead merged commit 8feddff into openwallet-foundation:main Aug 16, 2021
@dbluhm dbluhm deleted the refactor/outbound-queue branch September 17, 2022 14:35
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