Skip to content

Kafka task minimum message time#3035

Merged
gianm merged 2 commits intoapache:masterfrom
dclim:kafka-task-minimum-message-time
May 31, 2016
Merged

Kafka task minimum message time#3035
gianm merged 2 commits intoapache:masterfrom
dclim:kafka-task-minimum-message-time

Conversation

@dclim
Copy link
Copy Markdown
Contributor

@dclim dclim commented May 28, 2016

KafkaIndexTask support for a minimumMessageTime parameter and changes to KafkaSupervisor to utilize this parameter. Allows users to specify a threshold for how old messages can be until they are dropped; this helps to prevent segment allocation conflicts when running a realtime and batch pipeline concurrently.

Resolves the issues discussed in #3029.

@dclim dclim added this to the 0.9.1 milestone May 28, 2016
@dclim
Copy link
Copy Markdown
Contributor Author

dclim commented May 28, 2016

@gianm @himanshug implemented as discussed in #3029; if you have time to review this in the near future we can include it in 0.9.1 - I think the changes are pretty straightforward (and yes I did end up including the minimumMessageTime as part of the sequenceName).

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

final ?

@nishantmonu51
Copy link
Copy Markdown
Member

👍 , LGTM, only minor comments #3035 (comment) & #3035 (comment)

@dclim
Copy link
Copy Markdown
Contributor Author

dclim commented May 31, 2016

@nishantmonu51 changes made, thanks for the review

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

IIRC the comment on isTaskCurrent says what goes into this, and so should be updated

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

nice catch

@gianm
Copy link
Copy Markdown
Contributor

gianm commented May 31, 2016

👍 assuming nothing hinky is going on here https://github.com/druid-io/druid/pull/3035/files#r65221314

@dclim
Copy link
Copy Markdown
Contributor Author

dclim commented May 31, 2016

@gianm confirmed that this pull request contains zero hinkies. Comments added.

@gianm
Copy link
Copy Markdown
Contributor

gianm commented May 31, 2016

👍 after travis, thanks @dclim

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants