The Wayback Machine - https://web.archive.org/web/20201101090718/https://github.com/mysqljs/mysql/pull/1469
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

added support for the reset connection packet #1469

Open
wants to merge 3 commits into
base: master
from

Conversation

@ben-page
Copy link

@ben-page ben-page commented Jul 6, 2016

This PR adds support for the COM_RESET_CONNECTION packet, including documentation and tests.

The issue is here #780

@ben-page
Copy link
Author

@ben-page ben-page commented Jul 6, 2016

The build appears to be failing because it's running against a version of MySQL that doesn't support COM_RESET_CONNECTION.

@dougwilson
Copy link
Member

@dougwilson dougwilson commented Jul 6, 2016

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 :)

@ben-page
Copy link
Author

@ben-page ben-page commented Jul 6, 2016

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 feature/reset-connection-packet branch?

@ben-page
Copy link
Author

@ben-page ben-page commented Jul 6, 2016

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.

@dougwilson
Copy link
Member

@dougwilson dougwilson commented Jul 6, 2016

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.

@dougwilson
Copy link
Member

@dougwilson dougwilson commented Jul 6, 2016

You want the PR created against the feature/reset-connection-packet branch ?

I'm fine if you want to make the PR to master. I can't see anything different from the branch except the change to the README. An alternative approach would be if you cherry picked my commit onto your branch, and then made a second comment with your additional changes after it.

@ben-page
Copy link
Author

@ben-page ben-page commented Jul 6, 2016

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.

@dougwilson
Copy link
Member

@dougwilson dougwilson commented Jul 6, 2016

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?

@ben-page
Copy link
Author

@ben-page ben-page commented Jul 7, 2016

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.

@dougwilson
Copy link
Member

@dougwilson dougwilson commented Jul 7, 2016

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).

@dougwilson dougwilson added the feature label Jul 7, 2016
@dougwilson dougwilson self-assigned this Jul 7, 2016
@dougwilson
Copy link
Member

@dougwilson dougwilson commented Jul 7, 2016

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...

dougwilson and others added 2 commits Apr 8, 2014
# Conflicts:
#	lib/Connection.js
#	lib/protocol/Protocol.js
added docs for reset
@ben-page ben-page force-pushed the ben-page:reset-connection branch from c133445 to 2fa5c9e Jul 7, 2016
@ben-page
Copy link
Author

@ben-page ben-page commented Jul 7, 2016

@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.

@sidorares
Copy link
Member

@sidorares sidorares commented Jul 7, 2016

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

@dougwilson
Copy link
Member

@dougwilson dougwilson commented Jul 7, 2016

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).

@sidorares
Copy link
Member

@sidorares sidorares commented Jul 7, 2016

@dougwilson I might be able to look at migrating travis config to use dockerized builds

@ben-page
Copy link
Author

@ben-page ben-page commented Jul 7, 2016

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.

@ben-page ben-page force-pushed the ben-page:reset-connection branch 9 times, most recently from 4286b44 to c43c187 Jul 7, 2016
@ben-page ben-page force-pushed the ben-page:reset-connection branch 4 times, most recently from efe8bcc to d445ca5 Jul 7, 2016
@ben-page
Copy link
Author

@ben-page ben-page commented Jul 7, 2016

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
@ben-page ben-page force-pushed the ben-page:reset-connection branch from d445ca5 to 3c29686 Jul 8, 2016
@dougwilson dougwilson force-pushed the mysqljs:master branch 2 times, most recently from 638d79d to 88bade0 Jun 29, 2017
@sedenardi
Copy link

@sedenardi sedenardi commented Oct 4, 2018

Hi there, I know this is an old issue, but I'm wondering what the probability of getting this merged in is? MariaDB supports COM_RESET_CONNECTION since 10.2.4, and this seems like a pretty low-risk feature to add. Would love to help in any way.

@dougwilson dougwilson force-pushed the mysqljs:master branch 2 times, most recently from 946727b to 37fbbdd Nov 16, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.