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

Extendable configuration #11

Merged
merged 5 commits into from
Aug 14, 2013
Merged

Extendable configuration #11

merged 5 commits into from
Aug 14, 2013

Conversation

eapache
Copy link
Contributor

@eapache eapache commented Aug 14, 2013

Affects #7

Adds ClientConfig, ProducerConfig and ConsumerConfig structs and use them in the appropriate constructors. This permits good configuration without overwhelming the constructor parameters (since unspecified values default to 0 which we handle sanely). It also lets us add more configuration options in the future without changing the API, which is a good thing.

Note that this changes the default behaviour in a few cases:

  • the client will not retry on a LEADER_NOT_AVAILABLE error unless you specify MetadataRetries > 0
  • the producer will not wait for extra acknowledgement from the broker unless you specify RequiredAcks != 0 (it always waits for the TCP acknowledgement).

@burke @fw42

CC @tobi

Evan Huus added 4 commits August 14, 2013 10:56
This lets us configure retry behaviour with sane defaults, and provides an easy
API-safe way to add further configuration later.
Also add ConfigurationError type for use reporting invalid configuration values.
@burke
Copy link
Contributor

burke commented Aug 14, 2013

This looks like a reasonable change, but I'm hesitant to add extra burden to clients of the API for the simple case of "I want to post a message". Would it make sense to pass them as pointers and intelligently default nil to a default config?

@eapache
Copy link
Contributor Author

eapache commented Aug 14, 2013

I don't think passing nil is much easier than passing Config{}?

@burke
Copy link
Contributor

burke commented Aug 14, 2013

sarama.SomethingConfig{}, in calling code. I'm a lazy typist, and it's easier for the simple-case user to not have to know about a configuration type to assert that they're not doing anything that needs configuration.

Also, I left my phone at home today so I'm not getting github push notifications. Sorry for the delays.

@eapache
Copy link
Contributor Author

eapache commented Aug 14, 2013

Taking http://golang.org/pkg/testing/quick/ as canonical, you win this round :)

@burke
Copy link
Contributor

burke commented Aug 14, 2013

LGTM. :shipit:

eapache added a commit that referenced this pull request Aug 14, 2013
@eapache eapache merged commit 722c9b2 into master Aug 14, 2013
@eapache eapache deleted the extendable_configuration branch August 14, 2013 19:24
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