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

Change how Kafka is configured for collector and ingester #390

Merged
merged 2 commits into from
May 3, 2019
Merged

Change how Kafka is configured for collector and ingester #390

merged 2 commits into from
May 3, 2019

Conversation

objectiser
Copy link
Contributor

@objectiser objectiser commented May 3, 2019

The Kafka options were changed in Jaeger 1.11, to use specific properties for consumer and producer sides. The existing shared properties have been deprecated.

With the shared properties, it was possible for have a simplified streaming CR that only defined the kafka properties in storage and/or ingester components - however now the properties are specific to the producer/consumer, they have to be explicitly listed in the relevant component's options.

Unfortunately there is no way to make this backward compatible, as otherwise unexpected properties are passed to the collector. So this will need to be a breaking change - although only affects the streaming stragegy and kafka options.

Tested using strimzi. Next step will be to provide some e2e tests for streaming.

Signed-off-by: Gary Brown gary@brownuk.com

Signed-off-by: Gary Brown <gary@brownuk.com>
@objectiser objectiser requested a review from jpkrohling May 3, 2019 11:44
Copy link
Contributor

@jpkrohling jpkrohling left a comment

Choose a reason for hiding this comment

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

Once the unit tests are fixed, this is ready to be merged.

collector:
options:
kafka: # <1>
producer:
Copy link
Contributor

Choose a reason for hiding this comment

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

This is the strongest argument in favor of strong typing for CLI options, IMO

README.adoc Outdated
@@ -262,6 +269,8 @@ spec:
<1> Identifies the kafka configuration used by the collector, to produce the messages, and the ingester to consume the messages
<2> The deadlock interval can be disabled to avoid the ingester being terminated when no messages arrive within the default 1 minute period

NOTE: A Kafka environment can be configured using link:https://strimzi.io/[Strimzi's Kafka operator].
Copy link
Contributor

Choose a reason for hiding this comment

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

Signed-off-by: Gary Brown <gary@brownuk.com>
@codecov
Copy link

codecov bot commented May 3, 2019

Codecov Report

Merging #390 into master will decrease coverage by <.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #390      +/-   ##
==========================================
- Coverage   89.72%   89.71%   -0.01%     
==========================================
  Files          64       64              
  Lines        3094     3092       -2     
==========================================
- Hits         2776     2774       -2     
  Misses        216      216              
  Partials      102      102
Impacted Files Coverage Δ
pkg/deployment/collector.go 100% <100%> (ø) ⬆️
pkg/deployment/ingester.go 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 945569d...9dd5455. Read the comment docs.

@objectiser objectiser merged commit fc63d2b into jaegertracing:master May 3, 2019
@objectiser objectiser deleted the streaming branch May 3, 2019 13:08
@pavolloffay pavolloffay added the enhancement New feature or request label May 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants