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

Initialize partition owner with consumerId #178

Merged
merged 1 commit into from
Mar 20, 2015

Conversation

ismriv
Copy link
Contributor

@ismriv ismriv commented Mar 18, 2015

This avoids a race condition in checkPartitionOwnership, where another instance may create the node initialized to null, so the node will exist, it will try to get its contents but will fail in the comparison consumerId !== data.toString() since data may be null.

@haio
Copy link
Member

haio commented Mar 19, 2015

Hi, what issue will this pr solve?

@ismriv
Copy link
Contributor Author

ismriv commented Mar 19, 2015

The following error:

TypeError: Cannot call method 'toString' of undefined
    at /activity-processor/node_modules/kafka-node/lib/zookeeper.js:395:53
    at /activity-processor/node_modules/kafka-node/node_modules/node-zookeeper-client/index.js:165:26
    ...

@haio
Copy link
Member

haio commented Mar 19, 2015

how to reproduce this error?

@ismriv
Copy link
Contributor Author

ismriv commented Mar 19, 2015

Having a few instances running, and restarting one of them to trigger the rebalancing mechanism. It's a race condition, so it may be tricky to reproduce, but if you try this a few times it will finally happen.

@ismriv
Copy link
Contributor Author

ismriv commented Mar 19, 2015

Any red flags?

@haio
Copy link
Member

haio commented Mar 20, 2015

Still can't reproduce it, but this a improvement logically, so I think it can be merged.

haio added a commit that referenced this pull request Mar 20, 2015
Initialize partition owner with consumerId
@haio haio merged commit fb89e00 into SOHU-Co:master Mar 20, 2015
@ismriv
Copy link
Contributor Author

ismriv commented Mar 20, 2015

Brilliant! Thanks.

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

2 participants