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

Added retry for sql statement execution failure #123

Merged
merged 5 commits into from
May 10, 2023

Conversation

kaisecheng
Copy link
Contributor

@kaisecheng kaisecheng commented Apr 2, 2023

jdbc-integration 5.4.1 has changed the way of using the connection pool. Since then, the sql statement execution does not retry for any failure.

This PR adds a retry loop using the new settings statement_retry_attempts and statement_retry_attempts_wait_time to retry the failed statement execution.

Setting Default Value Comment
statement_retry_attempts 1 Maximum number of times to try running the statement
statement_retry_attempts_wait_time 0.5 Number of seconds to sleep between statement execution

Fix: #124

@kaisecheng kaisecheng marked this pull request as ready for review May 4, 2023 16:56
@kaisecheng kaisecheng requested a review from andsel May 9, 2023 14:23
Copy link
Contributor

@andsel andsel left a comment

Choose a reason for hiding this comment

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

LGTM

tried shutting down and tearing up a local DB between the statement_retry_attempts_wait_time and worked as expected. Good work.

Maybe a nitpick, instead of statement_retry_attempts_wait_time would be better statement_retry_attempts_interval. I think that amongst the plugin's code base the interval suffix is more frequently used than wait_time.
But feel free to skip the suggestion :-)

@kaisecheng
Copy link
Contributor Author

instead of statement_retry_attempts_wait_time would be better statement_retry_attempts_interval

Thanks for the review and suggestion. There is a connection_retry_attempts_wait_time in existing jdbc config, so I keep it the same naming convention.

@kaisecheng kaisecheng merged commit 7df0eae into logstash-plugins:main May 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add retry to failure sql execution
3 participants