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

#733 driver implemets the sql/driver/ConnBeginTx interface #779

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

albertlockett
Copy link
Contributor

@jkaflik
Copy link
Contributor

jkaflik commented Dec 12, 2022

Hi @albertlockett

Thanks for submitting this PR.
Did you check if otelsql is able to support SQL drivers that do not implement ConnBeginTx interface? From our point of view, it makes more sense to keep our API away from fake doors.

@jkaflik jkaflik added the stale requires a follow-up label Dec 16, 2022
@albertlockett
Copy link
Contributor Author

@jkaflik I will check w/ otelsql maintainers and follow up. Do you think it makes sense though to implement ConnBeginTx as driver.Conn's Begin method is deprecated?
https://pkg.go.dev/database/sql/driver#Conn

@jkaflik
Copy link
Contributor

jkaflik commented Mar 9, 2023

@albertlockett thanks for giving me a reference. Indeed, it seems we should deprecate ConnBeginTx interface.
I think we can proceed with your PR. Can we experiment by removing Begin function?

@jkaflik jkaflik added enhancement and removed discuss blocked stale requires a follow-up labels Mar 9, 2023
@albertlockett
Copy link
Contributor Author

We could experiment by removing the Begin function. I'm not sure if there are other reasons to keep it around for backwards compatibility?

Also, it looks like otelsql will begin supporting the driver.Conn interface if/when this PR merges:
XSAM/otelsql#153

@jkaflik jkaflik added the stale requires a follow-up label Aug 2, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement stale requires a follow-up
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants