-
-
Notifications
You must be signed in to change notification settings - Fork 197
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Added a few minor comments/suggestions.
.gitignore
Outdated
*.iml | ||
application.conf |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Might want to add a linefeed at the end...
kafka-lag-exporter/README.md
Outdated
* Add `kafka-client-timeout` config. | ||
* Tune retry and timeout logic of Kafka admin client and consumer | ||
* Use backoff strategy restarting offset collection logic when transient runtime exceptions are encountered | ||
* Terminate when prometheus http server can't start (i.e. port can't be bound) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider "Prometheus HTTP server..."
@@ -1,9 +1,9 @@ | |||
<configuration> | |||
<variable name="KAFKA_LAG_EXPORTER_LOG_LEVEL" value="${KAFKA_LAG_EXPORTER_LOG_LEVEL:-INFO}" /> | |||
<variable name="KAFKA_LAG_EXPORTER_LOG_LEVEL" value="${KAFKA_LAG_EXPORTER_LOG_LEVEL:-DEBUG}" /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm curious if you intended to use DEBUG
as the default or was this left over from some testing?
topicPartitions.map(tp => tp -> Measurements.Single(offsets.get(tp.asKafka).toLong,now)).toMap | ||
} | ||
|
||
/** | ||
* Get last committed Consumer Group offsets for all group topic partitions. When a topic partition has no matched | ||
* Consumer Group offset then a default offset of 0 is provided. | ||
* @param groups A list of Consumer Groups to request offsets for. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason you removed this Scaladoc @param
entry?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just consistency. I didn't have it for the other methods. I've updated the description.
Thanks for the review @deanwampler ! :) |
Fixes #6
PR features: