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

[kafka] YAML configuration template versionning #340

Merged
merged 1 commit into from Sep 9, 2016

Conversation

Projects
None yet
2 participants
@degemer
Copy link
Member

commented Aug 10, 2016

  • Use version attribute in kafka recipe to version, i.e.
    select the appriopriate YAML configuration file.
    Two versions are currently available
    • 1 (Default): Legacy YAML configuration file, compatible with
      Kafka < 0.8.2.
    • 2: Required for Kafka > 0.8.2, use the YAML configuration file
      introduced by DataDog/dd-agent#2079

@degemer degemer force-pushed the quentin/multiple-kafka-version branch 2 times, most recently from 657ffa3 to 64292fa Aug 10, 2016

@degemer degemer removed the in progress label Aug 10, 2016

# * `version` (optional)
# Select the appropriate configuration file template. Available options are:
# * `1` (Default, Kafka < 8.2).
# * `2` (Kafka >= 8.2).

This comment has been minimized.

Copy link
@olivielpeau

olivielpeau Aug 11, 2016

Member

nitpick: currently kafka is versioned with a leading 0., so it'd be 0.8.2

This comment has been minimized.

Copy link
@degemer

degemer Aug 11, 2016

Author Member

😵

is_jmx: true

# Metrics collected by this check. You should not have to modify this.
conf:

This comment has been minimized.

Copy link
@olivielpeau

olivielpeau Aug 11, 2016

Member

Could you include the changes that have been made to the default config on dd-agent/master?

https://github.com/DataDog/dd-agent/blob/5.8.5/conf.d/kafka.yaml.example should have all the latest changes

This comment has been minimized.

Copy link
@degemer

degemer Aug 11, 2016

Author Member

Done.

@olivielpeau

This comment has been minimized.

Copy link
Member

commented Aug 11, 2016

Thanks @degemer! Added in my 2 cents.

Overall I'm fine with this approach of using a version of 1 or 2, the comments you've added in the kafka recipe are clear enough IMHO.

@olivielpeau olivielpeau added this to the 2.6.0 milestone Aug 11, 2016

[kafka] YAML configuration template versionning
* Use `version` attribute in `kafka` recipe to version, i.e.
  select the appriopriate YAML configuration file.
  Two versions are currently available
  * `1` (Default): Legacy YAML configuration file, compatible with
    Kafka < 0.8.2.
  * `2`: Required for Kafka > 0.8.2, use the YAML configuration file
  introduced by DataDog/dd-agent#2079

@degemer degemer force-pushed the quentin/multiple-kafka-version branch from 64292fa to 773eebf Aug 11, 2016

@degemer

This comment has been minimized.

Copy link
Member Author

commented Aug 11, 2016

Thanks for the review @olivielpeau ! I updated it with the latest config file.

@degemer

This comment has been minimized.

Copy link
Member Author

commented Aug 11, 2016

Related PR: #264, #257
Related issue: #275

@olivielpeau

This comment has been minimized.

Copy link
Member

commented Aug 12, 2016

LGTM, thanks @degemer!

@olivielpeau olivielpeau added the ready label Aug 12, 2016

@olivielpeau olivielpeau merged commit bc890a5 into master Sep 9, 2016

2 checks passed

ci/circleci Your tests passed on CircleCI!
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@olivielpeau olivielpeau deleted the quentin/multiple-kafka-version branch Sep 9, 2016

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.