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

input/sqlserver: Add service and save connection pools #8596

Merged
merged 6 commits into from
Mar 29, 2021
Merged

input/sqlserver: Add service and save connection pools #8596

merged 6 commits into from
Mar 29, 2021

Conversation

mjiderhamn
Copy link
Contributor

@mjiderhamn mjiderhamn commented Dec 18, 2020 •
edited by sjwang90
Loading

Required for all PRs:

  • Signed CLA.
  • Associated README.md updated.
  • Has appropriate unit tests.

Since the issue persists while #6713 was closed by author @nwneisen , I'm opening up a new PR to fix #5898.

@helenosheaa helenosheaa added area/sqlserver feature request Requests for new plugin and for new features to existing plugins labels Dec 21, 2020
@helenosheaa
Copy link
Member

Thanks for reopening @mjiderhamn is this something you or @nwneisen are currently working on?

@mjiderhamn mjiderhamn force-pushed the reuse-sql-server-connections branch from efeff95 to 40e9c99 Compare December 21, 2020 20:54
@mjiderhamn mjiderhamn changed the base branch from release-1.16 to master December 21, 2020 20:54
@mjiderhamn mjiderhamn force-pushed the reuse-sql-server-connections branch 2 times, most recently from cddca7a to 4c9d327 Compare December 21, 2020 21:29
@mjiderhamn mjiderhamn force-pushed the reuse-sql-server-connections branch from 4c9d327 to 95cde44 Compare December 21, 2020 21:43
@mjiderhamn
Copy link
Contributor Author

I'd be willing to try to take this to a mergeable state.

I have rebased onto master and made CircleCI happy. Anything else you're thinking about @helenosheaa ?

@helenosheaa
Copy link
Member

@mjiderhamn sounds good, that's great! Nothing currently on my side.

@mjiderhamn
Copy link
Contributor Author

Any news on getting this merged, @helenosheaa

@helenosheaa
Copy link
Member

@mjiderhamn it looks like it has conflicts with master, if you can address those.

After that it needs to be reviewed, I'll take a look and request another reviewer :)

@helenosheaa helenosheaa self-assigned this Feb 1, 2021
Merge branch 'master' into reuse-sql-server-connections
55d97e6 
# Conflicts:
#	plugins/inputs/sqlserver/sqlserver_test.go
@mjiderhamn mjiderhamn force-pushed the reuse-sql-server-connections branch from 14973f8 to 55d97e6 Compare February 2, 2021 08:57
@mjiderhamn
Copy link
Contributor Author

@helenosheaa master conflict resolved

@helenosheaa helenosheaa requested a review from ssoroka February 2, 2021 15:04
@helenosheaa
Copy link
Member

@denzilribeiro or @Trovalo would either of you mind taking a look at this?

@sjwang90
Copy link
Contributor

sjwang90 commented Mar 5, 2021

@avinash-nigam would you or anyone else on your team be able to take a review on this PR?

masree
masree reviewed Mar 23, 2021
View reviewed changes
plugins/inputs/sqlserver/sqlserver.go
DatabaseType string `toml:"database_type"`
IncludeQuery []string `toml:"include_query"`
ExcludeQuery []string `toml:"exclude_query"`
pools []*sql.DB
Copy link
Contributor

@masree masree Mar 23, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

AFAIK, for connection pooling, we usually have a default pool size and add connection to pool if the number of connections is less than pool size. was there a specific reason of not having that mechanism? Is it safe to have any number of pools in the list? I think it might be ok considering it depends on the number of connection string from Servers []string toml:"servers". However would like to understand your thinking.

@masree
Copy link
Contributor

masree commented Mar 23, 2021

Looks good to me!

ivorybilled
ivorybilled reviewed Mar 25, 2021
View reviewed changes
Copy link
Contributor

@ivorybilled ivorybilled left a comment

Choose a reason for hiding this comment

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

Looks good! Would be curious to better understand how masree's comment might effect this but other than that this seems to make sense.

@mjiderhamn
Copy link
Contributor Author

I did leave a reply to @masree , which is marked as "Pending" :-/

I wrote

From the docs it seems to me you can only set max open vs idle, so I'm not sure what you mean with default pool size?

The idea is that each pool will only ever have 1 connection in them, since there is no concurrency involved. Yes, there should be 1 pool per connection string/server. The goal is to avoid opening and closing connections over and over for each interval, the way it currently does.

@helenosheaa
Copy link
Member

@mjiderhamn could you address the merge conflicts again, then we can merge. Thanks for your work on this.

@masree
Copy link
Contributor

masree commented Mar 26, 2021

I did leave a reply to @masree , which is marked as "Pending" :-/

I wrote

From the docs it seems to me you can only set max open vs idle, so I'm not sure what you mean with default pool size?
The idea is that each pool will only ever have 1 connection in them, since there is no concurrency involved. Yes, there should be 1 pool per connection string/server. The goal is to avoid opening and closing connections over and over for each interval, the way it currently does.

Sorry, I meant to say if we need to set max open and idle connections. Thanks for clarifying.

@mjiderhamn mjiderhamn force-pushed the reuse-sql-server-connections branch 2 times, most recently from 6df2543 to bfcc4e8 Compare March 27, 2021 10:08
@mjiderhamn mjiderhamn force-pushed the reuse-sql-server-connections branch 2 times, most recently from bbb8795 to 57fbada Compare March 27, 2021 11:25
@mjiderhamn
Copy link
Contributor Author

mjiderhamn commented Mar 27, 2021 •
edited
Loading

@helenosheaa I fixed the merge issues, but I don't understand what's up with CircleCI. If I run make tidy in an quay.io/influxdb/telegraf-ci:1.15.5 container it is successful.

Edit: Oh wait, you've switched to Go 1.16.

Merge branch 'master' into reuse-sql-server-connections
eb1fd8e 
# Conflicts:
#	go.mod
#	plugins/inputs/sqlserver/sqlserver.go
#	plugins/inputs/sqlserver/sqlserver_test.go
@mjiderhamn mjiderhamn force-pushed the reuse-sql-server-connections branch 3 times, most recently from 383867a to b67d713 Compare March 27, 2021 14:30
Merge branch 'master' into reuse-sql-server-connections
bdae599 
# Conflicts:
#	go.mod
#	plugins/inputs/sqlserver/sqlserver.go
#	plugins/inputs/sqlserver/sqlserver_test.go
@mjiderhamn mjiderhamn force-pushed the reuse-sql-server-connections branch from b67d713 to bdae599 Compare March 29, 2021 14:42
telegraf-tiger[bot]
telegraf-tiger bot reviewed Mar 29, 2021
View reviewed changes
Copy link
Contributor

@telegraf-tiger telegraf-tiger bot left a comment

Choose a reason for hiding this comment

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

Looks like new artifacts were built from this PR. Get them here!

Artifact URLs

@helenosheaa helenosheaa merged commit 871447b into influxdata:master Mar 29, 2021
jblesener pushed a commit to jblesener/telegraf that referenced this pull request Apr 18, 2021
@mjiderhamn @jblesener
input/sqlserver: Add service and save connection pools (influxdata#8596)
bdab182
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@masree masree masree left review comments

@ivorybilled ivorybilled ivorybilled left review comments

@telegraf-tiger telegraf-tiger[bot] telegraf-tiger[bot] left review comments

@ssoroka ssoroka ssoroka approved these changes

@helenosheaa helenosheaa helenosheaa approved these changes

Assignees

@helenosheaa helenosheaa

Labels
area/sqlserver feature request Requests for new plugin and for new features to existing plugins
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Reuse connections from connection pool in sqlserver input
7 participants
@mjiderhamn @helenosheaa @sjwang90 @masree @ssoroka @ivorybilled @nwneisen