trilogy-libraries / trilogy

Trilogy is a client library for MySQL-compatible database servers, designed for performance, flexibility, and ease of embedding.
MIT License
697 stars 68 forks source link

Return false instead of raising an error when connection is lost in Trilogy#ping method #145

Closed genya0407 closed 7 months ago

genya0407 commented 8 months ago

This pull request modifies the behavior of the Trilogy#ping method. Now, it returns false instead of raising an error when the connection is lost. This change will help users handle connection loss more gracefully and avoid unnecessary exceptions.

The test case test_trilogy_ping_after_close_returns_false suggests the original intention was to return false when the connection is lost. Additionally, Mysql2::Client#ping returns false instead of raising an error when the connection is lost. If Trilogy adopts the same behavior, it would make it easier for users to switch between the two libraries.

Furthermore, this change is expected to work without any issues in the trilogy_adapter of Rails, as Trilogy#ping method is only used in the following method: https://github.com/rails/rails/blob/dca72dab117e79ee75666927935a7a6a31dd5324/activerecord/lib/active_record/connection_adapters/trilogy_adapter.rb#L120-L124

jhawthorn commented 8 months ago

I don't think we should do this. As was discussed in #134 we deliver more information to the user by raising exceptions. It also allows potentially reusing the same error handling as we do for queries and other operations.

It's also not good for us here to return false without permanently closing the connection, as that could lead to an inconsistent state if further attempts are made to use it.

genya0407 commented 8 months ago

Thank you for your review. I’ve looked over the discussion in #134.

I concur that if the ping method raises an error, it can provide more detailed information to the user and ensure the connection is closed through existing exception handling. Upon further reflection, this proposed change might not be ideal, as any operation that relies on Trilogy#ping raising an error would not be able to handle a connection loss correctly.

In our specific use case, we have verified that the system functions correctly even when the ping method raises an error. So, I am comfortable with closing this PR. However, if many users are looking to transition from mysql2 to Trilogy but are hindered by the behavior of the ping method, it might be worthwhile to revisit this behavior change.

Additionally, it seems more accurate to rename the test case from test_trilogy_ping_after_close_returns_false to test_trilogy_ping_after_close_raises. If you agree, I will create a separate PR to implement this correction.

Thank you again for your time and consideration.

jhawthorn commented 7 months ago

Yes, I think that test name is just wrong (and as far as I can tell is as old as this project). A PR would be appreciated 😊

Though Trilogy is very similar to mysql2 (in part likely due to shared authorship) they aren't meant to be 1:1 drop-in compatible.