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

Fixed isAsync params usage for createTopics (#900) #1370

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

lamweili
Copy link

@lamweili lamweili commented Jan 3, 2020

Fixed #900 - The README documentation is wrong/unclear

@lamweili lamweili changed the title Fixed isAsync params usage for createTopics Fixed isAsync params usage for createTopics (#900) Jan 4, 2020
@lamweili lamweili force-pushed the BugFix/isAsync-params-usage-for-createTopics branch 4 times, most recently from b870e34 to fd08af2 Compare January 4, 2020 06:50
Async must default to false.
It is dangerous to use async=true for createTopics unless explicitly specified.

For running async calls, the subsequent steps (such as addTopics) will fail as the topics are yet to be created/in the progress of creation.
It is the nature of async to return immediately even before the createTopics is completed.

This fixes the regression from the fix 5fd3764.
@lamweili lamweili force-pushed the BugFix/isAsync-params-usage-for-createTopics branch from fd08af2 to b7b1953 Compare January 4, 2020 07:25
(To be consistent with test.highLevelProducer.js line 24)
@lamweili lamweili force-pushed the BugFix/isAsync-params-usage-for-createTopics branch from 2f88591 to 8526b0b Compare January 4, 2020 08:20
@lamweili
Copy link
Author

lamweili commented Jan 5, 2020

@hyperlink

I noticed that most of the pull requests have the following error from the Travis-CI test report:

  1. SSL Producer #send "before all" hook:
    Uncaught AssertionError: expected { '0': 14 } to have property '0' of 0 (got 14)
    at Assertion.fail (node_modules/should/lib/assertion.js:183:17)

Some examples are #1612.4 and #1612.7 of
https://travis-ci.org/SOHU-Co/kafka-node/builds/632764529?utm_medium=notification&utm_source=github_status

This error has got nothing to do with the pull requests, but rather a test suite issue.

I have patched test.producer.js to use uuid.v4() instead of Date.now() in the topic name, to reduce the risk of collision (8526b0b). Especially when Travis-CI runs multiple environments concurrently, it is highly likely that Date.now() might not be unique enough.

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.

The README documentation is wrong/unclear
1 participant