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

Fix race in broker version check #682

Merged
merged 1 commit into from
Jun 20, 2016
Merged

Fix race in broker version check #682

merged 1 commit into from
Jun 20, 2016

Conversation

eapache
Copy link
Contributor

@eapache eapache commented Jun 17, 2016

Move it until after we're sure we have a version to check against. Fixes a race
found by @kchaliki (#678 (comment)).

Also make the which-version-do-I-need part of the protocol interface where it
belongs.

Move it until after we're sure we have a version to check against. Fixes a race
found by kchaliki.

Also make the which-version-do-I-need part of the protocol interface where it
belongs.
@kchaliki
Copy link

@eapache thanks for the fix, I can confirm I no longer get a panic due to the nil pointer, however I still don't see a timestamp applied to my messages - does it work for you?

@eapache
Copy link
Contributor Author

eapache commented Jun 20, 2016

I have not been able to properly test this yet; it seems there are some broker-side settings that may need to be set in addition to just producing with the right message version?

@eapache eapache merged commit 1deca4b into master Jun 20, 2016
@eapache eapache deleted the fix-version-check-race branch June 20, 2016 14:59
@kchaliki
Copy link

Indeed, I will dig some more

@kchaliki
Copy link

Confirmed after running

kafka-topics.sh --zookeeper blah --alter --topic mytopic --config message.timestamp.type=LogAppendTime

I get timestamps. I think one small issue is they are millisecond timestamps and that is not accounted for correctly when parsing the response and calling time.Unix(block.Timestamp, 0)

@kchaliki
Copy link

Something like this does the trick

timestampNsec := block.Timestamp % 1000
timestampSec := block.Timestamp / 1000
timestamp := time.Unix(timestampSec, timestampNsec)

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.

2 participants