-
Notifications
You must be signed in to change notification settings - Fork 24
Load pickup repository via configuration #60
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
Load pickup repository via configuration #60
Conversation
Signed-off-by: jamshale <[email protected]>
Signed-off-by: jamshale <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I made similar changes in #55, but with a slightly different approach. I'm fine with merging this PR and I'll incorporate the changes.
I can then also do the update to zod-config to have strongly typed and validated config (i think if we allow multiple options for storage etc it will be good to be strict)
Signed-off-by: jamshale <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not yet too familiar with this codebase so will wait for @TimoGlastra or @genaris to approve, but went over the changes and things look good to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM, but I think the setup can be a bit simplified to make easier to use
PICKUP_TYPE=postgres | ||
PICKUP_STRATEGY=QueueOnly |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can simplify this setup by just specifying the pickup type, and choosing automatically the strategy depending on it. For postgres I think QueueOnly is the only one that actually makes sense, while we can use QueueAndDeliver for the in memory implementation.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've been working with the postgres pickup quite a bit and have a fork that allows live mode by only closing the database sessions when a websocket session is closed. I've tested it quite a bit in a clustered environment and it seems like all is well and websockets successfully change instances. I was going to create a separate PR to allow both delivery modes.
I'm curious what your concerns are for live mode with the postgres pickup repository? If there's a problem I'm not seeing? I think there would be an issue with stale sessions that would need to addressed for live mode but I haven't seen any other major problems.
Signed-off-by: jamshale <[email protected]>
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be good with merging as-is, changes look good and we can tweak in the future as/if needed
This pull request allows the mediator to load a pickup repository through environment variables and config values. It only currently allows the postgres pickup repository to be loaded but adding a loader for the dynamodb pickup should be straight forward.