-
Notifications
You must be signed in to change notification settings - Fork 30
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
replace synchronised with RetrantLock - so that its JDK 21 Virtual thread Project loom friendly. #110
base: master
Are you sure you want to change the base?
Conversation
@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
<#110 (comment)>,
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
> <#110 (comment)>,
> 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
<#110 (comment)>,
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.
Please let us know which features you would like to see implemented in the Redshift JDBC Driver. |
Description
Motivation and Context
Testing
Screenshots (if appropriate)
Types of changes
Checklist
mvn install
succeedsLicense