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.

Sign up for GitHub

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

Jump to bottom

Used Prepared Statements for GetChannel in clickhousereader #1414

Merged
merged 5 commits into from
Jul 28, 2022
Merged

Used Prepared Statements for GetChannel in clickhousereader #1414

merged 5 commits into from
Jul 28, 2022

Conversation

jshiwam
Copy link
Contributor

@jshiwam jshiwam commented Jul 16, 2022 •
edited
Loading

Added prepared statements for GetChannel(id string).

@welcome
Copy link

welcome bot commented Jul 16, 2022

Welcome to the SigNoz community! Thank you for your first pull request and making this project better. 🤗

@request-info
Copy link

request-info bot commented Jul 16, 2022

We would appreciate it if you could provide us with more info about this issue/pr!

@makeavish
Copy link
Member

@jshiwam How prepared statements are useful here?
They can be useful for mutation of table/db but not sure how they are useful here.

@jshiwam
Copy link
Contributor Author

jshiwam commented Jul 17, 2022

@makeavish yes you are right in this case it is not that useful. What I understood from the issue is that any query that uses an args needs to be moved to prepared statement. If this is not required then I will remove it and make the other changes.

@makeavish makeavish linked an issue Jul 18, 2022 that may be closed by this pull request
Move db calls to prepared statements with context #1353
Open
@makeavish
Copy link
Member

Got it, I didn't see the issue: #1353
Linked it.

@jshiwam
Copy link
Contributor Author

jshiwam commented Jul 18, 2022

@makeavish there are other functions that has to be changed according to this issue, should I wait for the feedback until this PR gets merged or this will be open and should I add the other changes incrementally on the same PR?. Just want to make sure the testing at your end is easier. How are we gonna go about it?

srikanthccv
srikanthccv reviewed Jul 27, 2022
View reviewed changes
Copy link
Member

@srikanthccv srikanthccv left a comment

Choose a reason for hiding this comment

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

Using transaction here seems like an unnecessarily complication to me. You would generally use transaction for in case of mutation like CREATE, UPDATE etc...

srikanthccv
srikanthccv reviewed Jul 27, 2022
View reviewed changes
Copy link
Member

@srikanthccv srikanthccv left a comment

Choose a reason for hiding this comment

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

The change can something simple like this.

stmt, err := r.localDB.Preparex(`SELECT id, created_at, updated_at, name, type, data data FROM notification_channels WHERE id=?`)
err = stmt.Get(&channel, idInt)

@jshiwam
Copy link
Contributor Author

jshiwam commented Jul 27, 2022

Yeah I was not sure whether to use transactions or not but used it in this case. Will make the changes and submit the PR.

@jshiwam jshiwam requested a review from prashant-shahi as a code owner July 27, 2022 20:09
@sonarqubecloud
Copy link

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@jshiwam
Copy link
Contributor Author

jshiwam commented Jul 27, 2022

@srikanthccv made the suggested changes. Please confirm. I might have to squash the commits. Did some rebasing and resolved many conflicts.

@ankitnayan ankitnayan merged commit e39d2f7 into SigNoz:develop Jul 28, 2022
@makeavish makeavish added this to the v0.10.1 milestone Aug 5, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@srikanthccv srikanthccv srikanthccv approved these changes

@makeavish makeavish Awaiting requested review from makeavish

@nityanandagohain nityanandagohain Awaiting requested review from nityanandagohain

@prashant-shahi prashant-shahi Awaiting requested review from prashant-shahi

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
v0.10.1
Development

Successfully merging this pull request may close these issues.

Move db calls to prepared statements with context
4 participants
@jshiwam @makeavish @srikanthccv @ankitnayan