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

Postgres support #108

Merged
merged 14 commits into from
Apr 3, 2023
Merged

Postgres support #108

merged 14 commits into from
Apr 3, 2023

Conversation

alek-github
Copy link
Contributor

@alek-github alek-github commented Feb 22, 2023

Description

Added postgres support to kafkaflow-retry-extensions based on the implementation for sql server. It's needed to enable using the package with postgres db. Decided to make it as similar as possible to sql code, to make sure it's aligned to the existing style & practices and does not cause any discrepancies. Further adjustments of extracting shared code could be done in separate PRs, after confirming it with the repo owners. All of the postgres adjustments (comparing to sql code/scripts) can be viewed by skipping first commit

How Has This Been Tested?

Run all unit tests and integration tests locally to make sure that new integration works. New integration is covered same way as sql server

Checklist

  • My code follows the style guidelines of this project
  • I have performed a self-review of my own code
  • I have added tests to cover my changes
  • I have made corresponding changes to the documentation

Disclaimer

By sending us your contributions, you are agreeing that your contribution is made subject to the terms of our Contributor Ownership Statement

@alek-github
Copy link
Contributor Author

Would appreciate Your review @luispfgarces @filipeesch @fernando-a-marins

@luispfgarces luispfgarces added the enhancement New feature or request label Feb 24, 2023
Copy link
Contributor

@luispfgarces luispfgarces left a comment

Choose a reason for hiding this comment

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

Implementation seems to be very alike the SQL Server implementation. Many of the things are duplicated, but I agree that this could not be accomplished without doing it (at least without a structural refactor).

I am leaving just 2 comments on minor things to solve, nothing that poses a big deal. Thank you @alek-talabat for your contribution. On my end, I'll approve once you fix those 👌

@luispfgarces
Copy link
Contributor

We need to merge #93 to fix the build pipeline (support for builds with forked repositories).

luispfgarces
luispfgarces previously approved these changes Feb 28, 2023
Copy link
Contributor

@luispfgarces luispfgarces left a comment

Choose a reason for hiding this comment

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

Thank you for your changes @alek-talabat. LGTM

@luispfgarces
Copy link
Contributor

@alek-talabat the PR #93 was merged, which will fix your build pipeline. Please merge from main branch.

@martinhonovais
Copy link
Contributor

Hi @alek-talabat,
First of all, many thanks for your contribution, we are very glad to review it.
Everything seemed good to me but I have some considerations:

  1. I feel that we could have a sample using the PostgresSql repo. This could help the PostgresSql users experimenting the repo with not too much effort.
  2. Last week we merged to main a feat that allows the dev to apply a retention policy for queues that have been done. Please take it into consideration in your PR as well. It will help devs to save repo space and money.
  3. Let us know if you did some API's tests using PostgreSql as a repo and if it went well.

@gsferreira
Copy link
Contributor

  • I feel that we could have a sample using the PostgresSql repo. This could help the PostgresSql users experimenting the repo with not too much effort.

@alek-talabat the sample can be a different PR.

And by the way, excellent contribution right here! 👏

# Conflicts:
#	README.md
@luispfgarces
Copy link
Contributor

@alek-talabat can you please check the build error?

Error: /home/runner/work/kafkaflow-retry-extensions/kafkaflow-retry-extensions/src/KafkaFlow.Retry.Postgres/RetryQueueDataProvider.cs(18,52): error CS0535: 'RetryQueueDataProvider' does not implement interface member 'IRetryDurableQueueRepositoryProvider.DeleteQueuesAsync(DeleteQueuesInput)' [/home/runner/work/kafkaflow-retry-extensions/kafkaflow-retry-extensions/src/KafkaFlow.Retry.Postgres/KafkaFlow.Retry.Postgres.csproj]

I think that the pull request that went Live before yours created a new method on the repository interface, which yours does not have (because you implemented it before this change).

Thank you! 🙏

@alek-github
Copy link
Contributor Author

alek-github commented Mar 31, 2023

Sorry for the delay, but changes from the most recent master are already included. Would appreciate Your reviews @martinhonovais @gsferreira to have this one closed

@luispfgarces luispfgarces merged commit 20b242d into Farfetch:main Apr 3, 2023
@alek-github alek-github deleted the postgres branch April 3, 2023 13:12
@luispfgarces luispfgarces moved this from Current to Done in KafkaFlow Retry roadmap Apr 3, 2023
fernando-a-marins added a commit that referenced this pull request Jun 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Development

Successfully merging this pull request may close these issues.

None yet

5 participants