Skip to content

Implement exponential backoff when creating a database connection #718

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

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

WouterGritter
Copy link

Avoid error spam when the SQL server is (temporarily) offline. Without this, multiple stacktraces are written to the console per second.

@WouterGritter
Copy link
Author

The existing createDataSource method would be cleaner without debug-logging every time we create a new SQL connection, as the connectionFactory could just be passed straight through to the new RetryingConnectionFactory. What do you think of this? Should I remove the debug log?

Copy link
Member

@TBlueF TBlueF left a comment

Choose a reason for hiding this comment

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

Please run ./gradlew spotlessApply and commit, to fix the build error :)


The existing createDataSource method would be cleaner without debug-logging every time we create a new SQL connection, as the connectionFactory could just be passed straight through to the new RetryingConnectionFactory. What do you think of this? Should I remove the debug log?

We could just move the debug-log into your new class. I think its useful to have and that way we could have both, the neater syntax and the log :)


Whats the behavior when starting/reloading bluemap with a broken/wrong database connection config? E.g. wrong connection-url / credentials?
I feel like retrying with a bad hostname or bad credentials would not be good, also we need to display the error in those cases to inform whats wrong. 🤔

@WouterGritter
Copy link
Author

I will do some testing with different invalid SQL configs soon and post them here in the comments.
However - I think it's an improvement to have exponential backoff in the scenarios you suggested. Right now it attempts to create a new connection multiple times per second, even when (an) invalid domain/credentials are provided.

End result is also still the same with this approach - after the fifth caught exception, it just throws it like the old implementation would. That stacktrace will provide you with whats wrong, e.g. connection refused (the IllegalStateException will never get thrown - if maxAttempts is 1 or greater anyways).

Maybe it's good to provide the exception message in each warning log after the caught SQL exceptions, just in case the reason for the exception is different than the last one (thats currently thrown).

@WouterGritter
Copy link
Author

I think we can close this PR @TBlueF. It's a lot more hassle than it's worth, at least for me. Some of the issues I saw:

  • The raised exceptions after the maxAttempts amount of attempts seems to be caught somewhere internally, and it isn't displaying. Probably some other retrying logic.
  • Now we're getting a warning spam, where its saying it'll retry in X seconds. Multiple threads are simultaneously attempting to make connections, leading to some retry-loops being ran in parallel.
  • Because it's blocking the thread when it can't create a connection, attempting to stop the Minecraft server while the SQL server isn't reachable will result in it retrying forever, or at least for a while. (Waiting for task 'preparing map 'world' update' to stop.. (0.0%))

I only wanted to fix the error spam since the connection to my SQL server isn't stable. I think this is definitely something worth looking at for BlueMap, but I'll just fork the project and ignore the exception once caught as a hack for my own usecase.

@WouterGritter WouterGritter marked this pull request as draft July 7, 2025 10:35
@TBlueF
Copy link
Member

TBlueF commented Jul 7, 2025

I agree that an error spam should be fixed/limited ..
But ofc this can't be at the cost of not showing errors at all anymore or improperly..
I will take this onto my TODO to look at 👍

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.

2 participants