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

override default healthcheck support #35

Closed
wants to merge 7 commits into from

Conversation

momo-p
Copy link

@momo-p momo-p commented Apr 21, 2023

I'm currently using this crate to communicate with my manticore server, even though it's compatible wit MySQL but for some reason it's not support SELECT version() which make the default healthcheck always throw error.

So I'm adding a feature to override the default healthcheck function.

@robjtede
Copy link
Collaborator

robjtede commented Jul 8, 2023

Is there any reason not to just change the current health check query to SELECT 1?

@momo-p
Copy link
Author

momo-p commented Jul 8, 2023

Is there any reason not to just change the current health check query to SELECT 1?

Maybe I over-engineered it a little bit, but I think it could be for future-proofing.

Like in case query log enabled for debugging and you don't want it full of SELECT 1 in the logs and your database support it you could swap it with the .ping() call

@robjtede
Copy link
Collaborator

robjtede commented Jul 8, 2023

Even better, would be nice if ping returned Result<(), Error> then it would be a no-brainer to switch to.

For now I'm tempted to change it to SELECT 1.

@momo-p
Copy link
Author

momo-p commented Jul 8, 2023

the current ping() method in the mysql crate return bool so I believe it would took a few more step.

what I really concern at that time (sorry I wrote those at 3AM :D) is that give it a little more flexibility, who knows if some future sql-like database in the future support COM_PING or their parser accept SELECT 1;, so I add it just-in-case needed

@robjtede
Copy link
Collaborator

robjtede commented Aug 3, 2023

Pushed a change that:

  • uses more standard type naming
  • changes the signature of HealthCheckFn to a function pointer
    • this is equivalent in the fns it accepts to what you'd written before with the &'static bound on health_check_fns param to with_custom_health_check but avoids the Arc overhead

@robjtede robjtede requested a review from outersky August 3, 2023 05:16
@robjtede
Copy link
Collaborator

robjtede commented Aug 3, 2023

@outersky curious what you think. I'm hesitant but seems that my PR blackbeam/rust-mysql-simple#355 to change the .ping() signature is slow going so might be a nice QoL change, for now at least. (My feeling is that we necessarily push enough major versions that it could be removed without to much hassle in the future.)

@robjtede
Copy link
Collaborator

robjtede commented Aug 3, 2023

Also for the CI fail: We'll rebase this after #37 is merged.

@robjtede
Copy link
Collaborator

robjtede commented Jun 7, 2024

superseded by #38

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