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

feat(inputs.sql): Add 'disconnected_servers_behavior' field in the configuration #13289

Merged
merged 4 commits into from
May 22, 2023

Conversation

neelayu
Copy link
Contributor

@neelayu neelayu commented May 19, 2023

Required for all PRs

resolves #13282

@telegraf-tiger telegraf-tiger bot added area/sql feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins labels May 19, 2023
Copy link
Contributor

@powersj powersj left a comment

Choose a reason for hiding this comment

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

Thank you for the PR! I have put some thoughts/questions below. My main purpose of the changes is to ensure the logic is as streamlined as possible should we add additional retry logic in the future.

plugins/inputs/sql/sql.go Outdated Show resolved Hide resolved
plugins/inputs/sql/sql.go Outdated Show resolved Hide resolved
plugins/inputs/sql/sql.go Outdated Show resolved Hide resolved
plugins/inputs/sql/sql.go Outdated Show resolved Hide resolved
plugins/inputs/sql/sql.go Outdated Show resolved Hide resolved
@neelayu
Copy link
Contributor Author

neelayu commented May 19, 2023

Thanks for reviewing. I agree with all of your suggestions, it makes sense that prepareStatements doesnt need to check for serverConnected status and the variable statementsPrepared is not needed.

I was experimenting with preparedStatments, and in fact I found that we dont even need to ping the server. The PrepareContext method takes care of creating new connection(or reuse) old ones if needed. However, our approach seems more logical and help if we want to incorporate new features in the future.

@telegraf-tiger
Copy link
Contributor

Copy link
Contributor

@powersj powersj left a comment

Choose a reason for hiding this comment

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

Thank you!

@powersj powersj added the ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review. label May 19, 2023
Copy link
Member

@srebhan srebhan 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 to me. Thanks for the nice feature @neelayu!

@srebhan srebhan merged commit 2476640 into influxdata:master May 22, 2023
@srebhan srebhan added this to the v1.27.0 milestone Jun 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/sql feat Improvement on an existing feature such as adding a new setting/mode to an existing plugin plugin/input 1. Request for new input plugins 2. Issues/PRs that are related to input plugins ready for final review This pull request has been reviewed and/or tested by multiple users and is ready for a final review.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

SQL Input plugin crashes at startup if server is unreachable
3 participants