amazon-redshift-jdbc-driver
amazon-redshift-jdbc-driver copied to clipboard
replace synchronised with RetrantLock - so that its JDK 21 Virtual thread Project loom friendly.
Description
Motivation and Context
Testing
Screenshots (if appropriate)
Types of changes
- [ ] Bug fix (non-breaking change which fixes an issue)
- [x] New feature (non-breaking change which adds functionality)
Checklist
- [x] Local run of
mvn install
succeeds - [x] My code follows the code style of this project
- [ ] My change requires a change to the Javadoc documentation
- [ ] I have updated the Javadoc documentation accordingly
- [ ] I have read the README document
- [ ] I have added tests to cover my changes
- [ ] All new and existing tests passed
- [ ] A short description of the change has been added to the CHANGELOG
License
- By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
@bhvkshah : Please review. Pull request for changes in this driver to make it "Project Loom" friendly.
Thanks for submitting the PR, @nitin1532 ! I will review it with the team. Could you please also add tests to ensure the changes work as expected and no existing functionality is broken?
Hey,
As I have just replaced synchronised with Retrant lock code, so existing tests should confirm its impact if any. Hence I didn’t add any new test case for it. Additionally I have cross verified the lines with latest postgre.jar on which this jdbc is originally based of and it looked fine to me.
Regards, Nitin Singhal
On Mon, 19 Feb 2024 at 2:26 pm, Bhavik Shah @.***> wrote:
Thanks for submitting the PR, @nitin1532 https://github.com/nitin1532 ! I will review it with the team. Could you please also add tests to ensure the changes work as expected and no existing functionality is broken?
— Reply to this email directly, view it on GitHub https://github.com/aws/amazon-redshift-jdbc-driver/pull/110#issuecomment-1952561044, or unsubscribe https://github.com/notifications/unsubscribe-auth/AKTJXVBCA2SV3G7CSDXB5UDYUNOKJAVCNFSM6AAAAABDNSP66SVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNJSGU3DCMBUGQ . You are receiving this because you were mentioned.Message ID: @.***>
Hi Bhavik,
Did you get a chance to review the changes done? Thanks.
Regards, Nitin Singhal
On Mon, 19 Feb 2024 at 8:07 pm, Nitin Singhal @.***> wrote:
Hey,
As I have just replaced synchronised with Retrant lock code, so existing tests should confirm its impact if any. Hence I didn’t add any new test case for it. Additionally I have cross verified the lines with latest postgre.jar on which this jdbc is originally based of and it looked fine to me.
Regards, Nitin Singhal
On Mon, 19 Feb 2024 at 2:26 pm, Bhavik Shah @.***> wrote:
Thanks for submitting the PR, @nitin1532 https://github.com/nitin1532 ! I will review it with the team. Could you please also add tests to ensure the changes work as expected and no existing functionality is broken?
— Reply to this email directly, view it on GitHub https://github.com/aws/amazon-redshift-jdbc-driver/pull/110#issuecomment-1952561044, or unsubscribe https://github.com/notifications/unsubscribe-auth/AKTJXVBCA2SV3G7CSDXB5UDYUNOKJAVCNFSM6AAAAABDNSP66SVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNJSGU3DCMBUGQ . You are receiving this because you were mentioned.Message ID: @.***>
Hi Nitin, I will review and have an update for you by next week. If all looks good, we can include this in one of the upcoming releases.
Hi,
I understand that you and the team are busy, but it has been over a month without any feedback on my open pull request. The lack of timely communication is frustrating.
If the pull request is rejected, I kindly request that you inform me promptly rather than leaving it in limbo. Additionally, I've noticed that the driver is not actively maintained and lacks support for the latest features. I appreciate your attention to this matter.
Looking forward to hearing back soon. Otherwise, I may consider withdrawing the pull request from GitHub.
Regards, Nitin Singhal
On Thu, 22 Feb 2024 at 8:53 PM, Bhavik Shah @.***> wrote:
Hi Nitin, I will review and have an update for you by next week. If all looks good, we can include this in one of the upcoming releases.
— Reply to this email directly, view it on GitHub https://github.com/aws/amazon-redshift-jdbc-driver/pull/110#issuecomment-1960294882, or unsubscribe https://github.com/notifications/unsubscribe-auth/AKTJXVCQSB2TPG7OBFXPYE3YU6V3TAVCNFSM6AAAAABDNSP66SVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTSNRQGI4TIOBYGI . You are receiving this because you were mentioned.Message ID: @.***>
Hi @nitin1532 thank you for understanding that the team is busy. I ran our test suite against the changes in your PR. However, the tests keep hanging with these changes, and not a single test passes successfuly. I do not want to outright reject your PR as the intent and the changes are useful, but it is evident that we will need to iterate a bit to ensure that these changes work as expected with our driver. I do not know when they team will be able to prioritize iterating on this PR.
Additionally, I've noticed that the driver is not actively maintained and lacks support for the latest features.
Please let us know which features you would like to see implemented in the Redshift JDBC Driver.