elixir-ecto / db_connection

Database connection behaviour
http://hexdocs.pm/db_connection/DBConnection.html
309 stars 112 forks source link

Document how the queueing system and deadlines work #176

Closed josevalim closed 5 years ago

cdegroot commented 5 years ago

Is this happening? The whole connection pooling/myxql thing is a bit of a hot mess, and we're trying to grasp how things are working, where the bugs are lurking, etcetera. This should not be a black box. I tried to read the ConnectionPool module but I can't figure it out.

josevalim commented 5 years ago

Yes, it will happen.

The whole connection pooling/myxql thing is a bit of a hot mess

In which way? Undocumented != hot mess. Which bugs?

I agree it is far from trivial to understand it but not understanding it does not mean it is a hot mess. Have the bugs been reported?

cdegroot commented 5 years ago

So, maybe the wording "hot mess" is too strong, but I don't think so. This particular library is a crucial component of the Phoenix/Ecto ecosystem, and should not be as undocumented and untested as it is. The "hot mess" comes from a number of factors - the lack of documentation and test coverage makes it impossible (well, at least very hard) to figure out what the code is doing, and I'm getting the feeling that there's some module naming that isn't exactly helpful either. Given a lack of knowledge about what the library is supposed to do and how we can expect it to behave, it is impossible to know whether behaviour we're seeing (like not handling keep alive pings as we expect it to happen) is due to bugs in the library (I think it is - it seems to not handle keep alives independently per connection but rather cycle through them and that full cycle might be too slow) or just bad expectations, hence writing bug reports becomes hard. We don't want to just bother people with misinterpretations and half truths.

For a cornerstone library like this one, the bar for code quality should be much, much higher and IMNSHO this stuff should be very well specified (there's something to say for the strictness of JDBC versus the "yeah, it's roughly this" of, say, the Python and Ruby equivalents :-) ). We would love to help but without design docs of this connection pooling (and why, say, Poolboy wasn't used and other decisions that shaped the code), it's hard to start.

josevalim commented 5 years ago

I agree the library is undocumented but can you please expand on how it is "untested"? Are you by any chance running only mix test and not the integration tests? For example, running MIX_ENV=ownership mix test --cover gives me 85% coverage and that is only one of the integration suites.

We don't want to just bother people with misinterpretations and half truths.

I would prefer a 1000 times the report you just gave about keep alives than a half-baked comment that the library is a hot mess and there are lurking bugs without giving any specific. The former gives something for us to work on, the latter just burns out maintainers.

cdegroot commented 5 years ago

I've been doing Open Source since ~1985 (yes - well before the term was invented :-)), so I use Strong Words when I need to, as a warning signal, and not to burn out maintainers. Note that in this case, it's not just this library, but the whole transition from MariaEx to Myxql, how are things supposed to work, and very qualified colleagues at work taking way too much time to puzzle over little undocumented details around keep alive, what happens exactly on connection cycling (especially w.r.t. outstanding transactions), and so on. Again, this stuff is too important to fly below the radar and currently it's pretty hard to figure things out. To me, sirens and red warning lights are going off.

And yes, I must say I totally missed the integration test directory when looking for "how is this stuff supposed to even work?" earlier this morning - my apologies for that. I guess the README could use some work - I'll submit a PR to that extent. Blind spots are always bigger than they appear or something and developer UX is also a thing - I guess I just expect all tests in test/ ;-).

I'll file a bug report for the keep alive stuff above and keep digging into how this library is supposed to work. But really, this one screams for a design document. And lots, lots more comments in the code.

josevalim commented 5 years ago

I've been doing Open Source since ~1985 (yes - well before the term was invented :-)), so I use Strong Words when I need to, as a warning signal, and not to burn out maintainers.

Linus has been doing Open Source for longer and he has recently recognized the impact that "Strong Words" have on open source communities. I recommend you to consider the same. You can talk about your intentions, I can talk about what I felt by your comments, make of that what you will. Just remember not having an intent to cause harm does not mean harm won't be caused.

Again, saying you are worried about the current status and you are looking for ways to help is great. Your original comment is far from it. And help is definitely appreciated. Maintaining those libraries are not fun. The reason we took on MyXQL is because nobody was maintaining Mariaex and nobody stepped in after multiple pleas, so we had to do it. Trust me, I would love to have my team work on something else and I would love if more people contributed to those important libs, so it doesn't fall entirely on us. In other words we are glad to provide more information whenever necessary. The alternative is for us to drop it and let people to their own devices.

I will lock this conversation because I believe everything that could be said here has been said. Again, I am glad to provide answers on technical aspects.