Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Fix client socket when closing and error handling #314

Merged
merged 2 commits into from
Feb 21, 2016

Conversation

carlessistare
Copy link
Collaborator

resolve #313 resolve #311 resolve #300
Fix on error handling after PR #262

@rphillips
Copy link

+1

@groundwater
Copy link

I had problems like #313 and #311 and this sesms to fix it.

Thank you for your hard work!

@carlessistare
Copy link
Collaborator Author

I'll try to do a PR adding and fixing old unit tests. It's becoming
impossible to know if you break something, every time that we add some
functionality.

Le lun. 18 janv. 2016 23:55, Jacob Groundwater notifications@github.com a
écrit :

I had problems like #313
#313 and #311
#311 and this sesms to fix
it.

Thank you for your hard work!


Reply to this email directly or view it on GitHub
#314 (comment).

@macnaughton
Copy link

+1! Fixed issue for me, many thanks!

@philippehenin
Copy link

Tested the correction. The Fix seems Ok.

@serega-ivanov
Copy link

+1

1 similar comment
@hhhonzik
Copy link

+1

@codyhazelwood
Copy link

+1 This fixes my issues. Thanks! It would be great to get this merged!

@johnfn
Copy link

johnfn commented Feb 9, 2016

THANK YOU SO MUCH

@Babacooll
Copy link

Could this be merged anytime soon? :)

@carlessistare
Copy link
Collaborator Author

@haio is there a way for this pr to be merge soon?

@nandanrao
Copy link

Using this branch, if I kill my broker while the client is connect, I get the following error (I'm not at all familiar with the codebase, but I assume it just needs another level of defensive checking):

Happy to play around on a branch, unless you feel like you know exactly what to do with this, @carlessistare?

if (!broker.socket || broker.socket.error) {
               ^
TypeError: Cannot read property 'socket' of undefined
    at Client.loadMetadataForTopics (/Users/nandanrao/Documents/Relink/parse/node_modules/kafka-node/lib/client.js:234:16)
    at RetryOperation._fn (/Users/nandanrao/Documents/Relink/parse/node_modules/kafka-node/lib/client.js:358:18)
    at RetryOperation.attempt (/Users/nandanrao/Documents/Relink/parse/node_modules/kafka-node/node_modules/retry/lib/retry_operation.js:56:8)
    at attemptRequestMetadata (/Users/nandanrao/Documents/Relink/parse/node_modules/kafka-node/lib/client.js:356:19)
    at Client.refreshMetadata (/Users/nandanrao/Documents/Relink/parse/node_modules/kafka-node/lib/client.js:352:5)
    at /Users/nandanrao/Documents/Relink/parse/node_modules/kafka-node/lib/highLevelConsumer.js:173:37
    at /Users/nandanrao/Documents/Relink/parse/node_modules/kafka-node/lib/highLevelConsumer.js:433:17
    at /Users/nandanrao/Documents/Relink/parse/node_modules/kafka-node/node_modules/async/lib/async.js:726:13

@carlessistare
Copy link
Collaborator Author

Thanks a lot for the input @nandanrao could you show how the procedure for killing the worker? I'd like to reproduce the error, before fixing.
Cheers

@nandanrao
Copy link

Yeah so I see the same problem in both of these cases:

  1. I have a running zookeeper, but no kafka broker connected. I run my node-kafka app, and it breaks in the above-stated way.

  2. I have a running zookeeper and a broker connected. I run my node-kafka app, and then I kill the broker. The node-kafka app breaks in the above-stated way.

In both cases, the node-kafka library successfully throws this error, which I catch:

BrokerNotAvailableError: Could not find a broker
        at new BrokerNotAvailableError (/Users/nandanrao/Documents/Relink/parse/node_modules/kafka-node/lib/errors/BrokerNotAvailableError.js:11:11)
        at Client.brokerForLeader (/Users/nandanrao/Documents/Relink/parse/node_modules/kafka-node/lib/client.js:495:32)
        at Client.loadMetadataForTopics (/Users/nandanrao/Documents/Relink/parse/node_modules/kafka-node/lib/client.js:232:23)
        at Client.send (/Users/nandanrao/Documents/Relink/parse/node_modules/kafka-node/lib/client.js:384:14)
        at Client.sendOffsetCommitRequest (/Users/nandanrao/Documents/Relink/parse/node_modules/kafka-node/lib/client.js:187:10)
        at autoCommit (/Users/nandanrao/Documents/Relink/parse/node_modules/kafka-node/lib/highLevelConsumer.js:489:21)
        at HighLevelConsumer.stop (/Users/nandanrao/Documents/Relink/parse/node_modules/kafka-node/lib/highLevelConsumer.js:588:10)
        at async.series.self.client.zk.getConsumersPerTopic.consumerPerTopicMap (/Users/nandanrao/Documents/Relink/parse/node_modules/kafka-node/lib/highLevelConsumer.js:274:22)

But soon after a couple of those are thrown, I get the broker-socket error, which crashes everything. Let me know if you have trouble reproducing!

@carlessistare
Copy link
Collaborator Author

Well observed, I'll fix this behaviour as soon as I can

@nandanrao
Copy link

You're saving my ass @carlessistare, thanks!!

@carlessistare
Copy link
Collaborator Author

Hi @nandanrao I admit that I could not get exactly the same error than you, but this should fix your issue f62191f
Does it make things right for you?

@springuper
Copy link

good job!

haio added a commit that referenced this pull request Feb 21, 2016
Fix client socket when closing and error handling
@haio haio merged commit 6957695 into SOHU-Co:master Feb 21, 2016
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Not reading from queue Consumer stops consuming after initial messages Consumer stops consuming