amazon-redshift-jdbc-driver icon indicating copy to clipboard operation
amazon-redshift-jdbc-driver copied to clipboard

replace synchronised with RetrantLock - so that its JDK 21 Virtual thread Project loom friendly.

Open nitin1532 opened this issue 1 year ago • 7 comments

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.

nitin1532 avatar Feb 17 '24 21:02 nitin1532

@bhvkshah : Please review. Pull request for changes in this driver to make it "Project Loom" friendly.

nitin1532 avatar Feb 17 '24 21:02 nitin1532

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?

bhvkshah avatar Feb 19 '24 14:02 bhvkshah

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: @.***>

nitin1532 avatar Feb 19 '24 20:02 nitin1532

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: @.***>

nitin1532 avatar Feb 22 '24 20:02 nitin1532

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.

bhvkshah avatar Feb 22 '24 20:02 bhvkshah

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: @.***>

nitin1532 avatar Mar 27 '24 09:03 nitin1532

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.

bhvkshah avatar Mar 27 '24 13:03 bhvkshah