Skip to content
This repository was archived by the owner on Aug 23, 2023. It is now read-only.

NotifierKafka: fix case when partition is empty (avoid hitting timeout) #1352

Merged
merged 1 commit into from
Jun 21, 2019

Conversation

fkaleo
Copy link
Contributor

@fkaleo fkaleo commented Jun 20, 2019

No description provided.

Copy link
Contributor

@robert-milan robert-milan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why is this still failing qa post build?

@@ -128,6 +128,10 @@ func (c *NotifierKafka) start() {
}
}

if startOffset < 0 {
log.Infof("kafka-cluster: empty partition, nothing to consume")
continue
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if we do that, we're not going to consume it if data gets pushed into this partition in the future

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please ignore my complete dumb-ness. I'll just fix the lastReadOffset in consumePartition like I originally said.

@fkaleo fkaleo force-pushed the nowait_when_no_messages2 branch from 252c083 to 6354efc Compare June 21, 2019 10:00
@fkaleo fkaleo force-pushed the nowait_when_no_messages2 branch from 6354efc to 16eaf01 Compare June 21, 2019 12:13
@fkaleo fkaleo merged commit 4d17e1f into master Jun 21, 2019
@fkaleo fkaleo deleted the nowait_when_no_messages2 branch June 21, 2019 13:09
lastReadOffset := startOffset - 1
var lastReadOffset int64
if startOffset < 0 {
log.Infof("kafka-cluster: empty partition")
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should probably mention which partition..

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants