Join GitHub today
GitHub is home to over 50 million developers working together to host and review code, manage projects, and build software together.
Sign upadded support for the reset connection packet #1469
Conversation
The build appears to be failing because it's running against a version of MySQL that doesn't support COM_RESET_CONNECTION. |
Hi @ben-page, thank you for your pull request! I already implemented this in the branch https://github.com/mysqljs/mysql/tree/feature/reset-connection-packet . Please if you can update your pull request to be based off that (or include my commit) instead of doing it again :) At the time, it was never merged because I don't want to add something I cannot test (because people will sometimes make PRs that break behavior, and it's really hard to know when this happens without tests). I know my branch is 2 years old now, and I would assume that Travis CI would be using a MySQL server version that supports this by now (but just forgot to check in those years). Would be be able to check this out for us as well? Once we can test it on at least one of our CI services (Travis CI or AppVeyor) then I'd be happy to merge my old reset connection branch and cut a release :) |
I did use that as a starting point, but it was 298(ish) commits behind master. So a lot had changed. Once I got everything working, I squashed the commits. I assumed you would not want to see the giant merge. But I'm fine with whatever you want. Do I understand you correctly? You want the PR created against the |
I ran the tests against Mysql 5.7.13.0 before creating the PR. Everything passes, including the new reset connection test. However, I only tested on Node.js 6.2.2. |
Hi @ben-page, as far as I can tell, the commit is basically what I wrote, but you changed the author, which to me, looks exactly like plagiarism. If you disagree that it is plagiarism, let me know what exactly is different. The only thing that sticks out to me is that you added some documentation, which even that looks like you copy and pasted the ping section, and forgot to change some of it from ping to reset. |
I'm fine if you want to make the PR to |
So you just want to see your commit in the mix? It was there, I promise. I just squashed it before making the PR. Most projects prefer PRs with a single commit. |
Yes, and we are one of those, but if this PR is nothing more than just the commit we already have, I'm just not quite sure what the PR is actually accomplishing? |
It's really counter productive to throw around words like plagiarism when someone is contributing toward your project. Your assessment that I simply copied your code and change the author, is wrong. Quite a few changes were required to bring it up to date. Additionally, I'm not taking your code and using it some project of mine, which would be plagiarism. I'm contributing it to your project. I'm give it to you. You can pull it and squash it and attribute it how you like. |
Thanks, @ben-page. I'm not sure why, but for some reason this conversation is going south quick, and I think I have contributed to that, I'm sorry. I'm going to let this sit for a week or so to let it chill out before revisiting so we can get back to a constructive conversation. In the meantime, if you can see what it would take to actually get this feature running on our CI, that would be great, because I cannot merge it if it doesn't run on CI (this is why it wasn't merged back 2 years ago). |
I'll be happy to simply merge whatever the contents of the PR are at that time once we get the tests passing and the documentation updated. I'll check back sooner, but I just wanted to be transparent if I'm not replying back right away but you see other activity. I also did look closer at the commits and I understand that you have moved around more that I initially thought. I have been struggling with mental issues lately and I'm sorry that you got caught in some of it, I'm very sorry... |
# Conflicts: # lib/Connection.js # lib/protocol/Protocol.js
added docs for reset
@dougwilson, I really appreciate your response. I see that having a the commit date from your original work, but my name as the author is suspicious. I'm actually not even sure why the squash worked out that way. So really sorry about that. I rebased my branch and cherry picked our commits. It now shows both commits. Let's move on in a more constructive fashion. As far as the CI goes, I think you just need to update MySQL. I can't seem to figure what version COM_RESET_CONNECTION was implemented in, but it was fairly recently. MySQL 5.7 is stable and has it. |
I think by default travis is using ubuntu 12.04 and mysql 5.5, I spent some time figuring out why behaviour is so different from my local tests |
So the Travis CI configuration for MySQL is just https://github.com/mysqljs/mysql/blob/master/.travis.yml#L57-L61 and I'm not sure if there is a way to specify the version of MySQL we want installed, but I have never really dug into the issue (especially since I had completely forgotten about the changes for 2 years :) ). I'll try to check it out when I have some time. Right now I'm trying to dig through about a week of backlogged issues and PRs across a ton of repositories, so may not be able to come back to this for a few days just from the limited time I usually have to spend :) That was why I was suggesting it, because I thought you may be able to spare the time to figure it out (and you can change the Travis CI configuration in the PR, since it's all done through that YAML file). |
@dougwilson I might be able to look at migrating travis config to use dockerized builds |
I'm not familiar with travis, but it looks to me (based on the config) it's testing against MariaDB not MySQL. MariaDB has not implemented COM_RESET_CONNECTION. I opened a ticket with them and they've assigned it to someone, but I have no idea when they will implement this. I didn't realize that I could tweak the Travis config like that. I'll see what I can figure out, but I doubt I can install MySQL from the .travis.yml. Since Ubuntu 12.04 installs MySQL 5.5, we can use the official MySQL APT Repository to stay up-to-date with the latest stable release. |
4286b44
to
c43c187
efe8bcc
to
d445ca5
OK, I figured out how to get Travis to use MySQL 5.7. Unfortunately, it doesn't support installing 5.7 as an addon or via it's built in apt support (the MySQL repo isn't whitelisted). So I was forced to write a script that manually remove MySQL 5.5 and install MySQL 5.7. Additionally, I had to update the 'dates-as-strings' test because it was using invalid dates like 0000-00-00 or 2013-00-00. MySQL 5.7 complained about them and that caused the test to fail. I kept the changes for supporting MySQL 5.7 and the changes for COM_RESET_CONNECTION as separate commits, because I wanted to make it easier for you to review them individually. Please let me know if you would like to me to do this differently. |
corrected 'dates as string' test
638d79d
to
88bade0
Hi there, I know this is an old issue, but I'm wondering what the probability of getting this merged in is? MariaDB supports |
946727b
to
37fbbdd
This PR adds support for the COM_RESET_CONNECTION packet, including documentation and tests.
The issue is here #780