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

[STORM-1991] Support auto.commit.interval in Kafka Client #1587

Closed
wants to merge 1 commit into from

Conversation

darionyaphet
Copy link
Contributor

STORM-1991 Support auto.commit.interval in Kafka Client

Support auto.commit.interval in new Kafka spout

@harshach
Copy link
Contributor

@hmcl can you review this.

@hmcl
Copy link
Contributor

hmcl commented Jul 26, 2016

@darionyaphet can you please clarify why this patch is needed? In my understanding it shouldn't be necessary to have any logic for auto commit interval. If one wishes to set the auto commit interval, one can do so using the kafka properties autocommit.interval.ms and autocommit.enable. These properties can be specified as in this example, by doing put("autocommit.interval.ms",200) and put("autocommit.enable",true)

The Kafka specific properties are then passed the KafkaConsumer here.

The commit timer is only used when in non auto commit mode. The 500 is only the initial delay. One can make this initial delay configurable, but it may not be of extreme importance to do so.

@darionyaphet
Copy link
Contributor Author

Hi @hmcl auto.commit.interval.ms is the interval to committed offset into zookeeper and it default value is 60 * 1000 (1S). I found there is a constant in KafkaSpoutConfig .Consumer , so I try to expose a method to config it. The commit time is 500ms in KafkaSpoutConfig ,actually they are the same .

@hmcl
Copy link
Contributor

hmcl commented Jul 26, 2016

@darionyaphet this kafka spout does not use zookeeper as the new Kafka API writes the offsets to a topic rather than to zookeeper. As I mentioned in the initial comment, this change is not necessary. Furthermore, all the kafka properties are supposed to go in a map, as illustrated in this example

At last, the commitTimer is not used in auto commit mode.

I would like to suggest that you close this PR.

@darionyaphet darionyaphet deleted the STORM-1991 branch July 27, 2016 02:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants