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

Support Node.js 13.x #759

Merged
merged 1 commit into from
Apr 5, 2020
Merged

Support Node.js 13.x #759

merged 1 commit into from
Apr 5, 2020

Conversation

sam-github
Copy link
Contributor

@sam-github sam-github commented Mar 6, 2020

Builds and passes tests on Ubuntu with 13.x.

Also fixes many deprecations. A few outstanding deprecations related to
integrating with the new async context tracking API remain, so
continuation-local-storage may (still) not work (have not checked).

Fixes: #733


This was a fairly mechanical fix, I'm not that familiar with the code or usage.

I noticed that npm test is passing but make is not, which concerned me, but it turns out the same is true for master. So, I'm still concerned :-), but it isn't a problem intoduced by this PR AFAICT.

package.json Outdated Show resolved Hide resolved
@sam-github
Copy link
Contributor Author

@iradul and perhaps @davidtrihy since you have C++ experience according to #628 (comment)

@iradul
Copy link
Collaborator

iradul commented Mar 15, 2020

Thank you for this PR!

@sam-github
Copy link
Contributor Author

I'll figure out the conflicts tomorrow and rebase.

src/binding.cc Outdated Show resolved Hide resolved
@skairali
Copy link

+1

Builds and passes tests on Ubuntu with 13.x.

Also fixes many deprecations.  A few outstanding deprecations related to
integrating with the new async context tracking API remain, so
continuation-local-storage may (still) not work (have not checked).

Fixes: #733
@sam-github sam-github changed the title Support Node.js 13.x and drop support for EOL Support Node.js 13.x Mar 16, 2020
@sam-github
Copy link
Contributor Author

Builds on <8.x now.

src/common.cc Show resolved Hide resolved
@csantanapr
Copy link

Thank you for this PR 👍

@iradul
Copy link
Collaborator

iradul commented Mar 19, 2020

Some e2e tests are failing on v13. I'll have investigate this.

@sam-github
Copy link
Contributor Author

@iradul I didn't find info on how to run the e2e tests, I'll take a shot at fixing issues if I know how to reproduce.

@IdanAdar
Copy link

@iradul can you help out here?

@iradul
Copy link
Collaborator

iradul commented Apr 2, 2020

@iradul I didn't find info on how to run the e2e tests, I'll take a shot at fixing issues if I know how to reproduce.

You first need to start local Kafka, you can use provided docker-compose -d up for that. Also, you may need to run it twice or so until Kafka is actually up. This is because sometimes Zookeeper boots slowly and then Kafka fails to load.

After you have Kafka running on localhost:9092, you just do make e2e to run the tests.

@iradul
Copy link
Collaborator

iradul commented Apr 5, 2020

I'll merge this since the e2e test issue (an empty string Buffer becomes null) is related to a change in v13 NodeJS/V8 code base and how it handles zero length Buffer. It's reported here.
I have a workaround that I'll commit soon.

@iradul iradul merged commit 22452f8 into Blizzard:master Apr 5, 2020
@sam-github sam-github deleted the fix-warnings branch April 6, 2020 03:43
@sam-github
Copy link
Contributor Author

Thanks for looking at this @iradul

@ntkumar1
Copy link

ntkumar1 commented Apr 6, 2020

@iradul When can we expect next version of rdkafka with node.js 13 support

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.

Error Make failed with exit code 2, Node 13
6 participants