Skip to content
This repository has been archived by the owner on Apr 5, 2022. It is now read-only.

[Sprint: 38] XD-2339a KafkaConsumer configs as module options #1288

Closed
wants to merge 2 commits into from

Conversation

ilayaperumalg
Copy link
Contributor

  • Remove external config properties for kafka consumer
  • Add module options for all the possible kafka consumer configurations

@ilayaperumalg ilayaperumalg changed the title XD-2239 KafkaConsumer configs as module options [Sprint: 38] XD-2239 KafkaConsumer configs as module options Nov 15, 2014
 - Remove external config properties for kafka consumer
 - Add module options for all the possible kafka consumer configurations
@ilayaperumalg ilayaperumalg changed the title [Sprint: 38] XD-2239 KafkaConsumer configs as module options [Sprint: 38] XD-2339 KafkaConsumer configs as module options Nov 15, 2014
@ilayaperumalg ilayaperumalg changed the title [Sprint: 38] XD-2339 KafkaConsumer configs as module options [Sprint: 38] XD-2339a KafkaConsumer configs as module options Nov 15, 2014

private int refreshLeaderBackOff = 200;

private String autoOffsetReset = "largest";

Choose a reason for hiding this comment

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

Is the switch here from "smallest" to "largest" intentional, in order to align the defaults within Kafka?

I think that such an alignment is generally desirable, but in this case, it is up to us to decide how we want new sources to behave (i.e. start at the end of the log or at the beginning of the log), and "smallest" for new sources would be a better strategy, IMO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, it is align with the default values that Kafka suggests. I can change it to "smallest". Could you elaborate why "smallest" value is desirable in this case?

Choose a reason for hiding this comment

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

I think that in this case we should align with https://github.com/spring-projects/spring-integration-kafka/blob/master/src/main/java/org/springframework/integration/kafka/core/KafkaConsumerDefaults.java

In particular, I find that "smallest" is better from a business standpoint because it causes new consumer groups to start at the beginning of the log, which is rather deterministic, rather than at the end, which isn't. This goes better with the semantic that a new source with a new groupId is a different view on the same set of data. Of course, these are just defaults, so users can override these values as they feel necessary.

Choose a reason for hiding this comment

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

Also, the value used to be "smallest" prior to 0.8, and it's been changed, in order to accomodate migration from 0.7 to 0.8 - I don't think that motivation applies to us.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The default value for consumer timeout in Kafka SI is set to use 5000 ms and the actual default is "-1" which makes the consumer wait indefinitely. Since we have module lifecycle, I believe setting it to "-1" makes sense. What do you think?

Choose a reason for hiding this comment

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

As per our discussion, we should avoid thread blocking.

@mbogoevici
Copy link

Merged 1d8a1fa

@mbogoevici mbogoevici closed this Nov 17, 2014
@ilayaperumalg ilayaperumalg deleted the XD-2339a branch February 18, 2015 20:03
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
2 participants