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-5901: Added Connect metrics specific to source tasks #3959

Closed
wants to merge 1 commit into from

Conversation

rhauch
Copy link
Contributor

@rhauch rhauch commented Sep 25, 2017

Added Connect metrics specific to source tasks, and builds upon #3864 and #3911 that have already been merged into trunk.

+ " must be greater than the minimum value " + min);
}
if (buckets < 1) {
throw new IllegalArgumentException("Must be at least 2 buckets");
Copy link
Contributor

Choose a reason for hiding this comment

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

The message doesn't seem to match the condition above.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch. Will fix shortly.

@wushujames
Copy link
Contributor

Can you update the docs as well? docs/ops.html

@rhauch rhauch changed the title KAFKA-5901 Added Connect metrics specific to source tasks KAFKA-5901: Added Connect metrics specific to source tasks Sep 25, 2017
@rhauch
Copy link
Contributor Author

rhauch commented Sep 26, 2017

@wushujames, I'll follow up with the docs change in a separate PR.

Copy link
Contributor

@ewencp ewencp left a comment

Choose a reason for hiding this comment

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

i think this is ready to go except for the one nit. i can also clean it up on commit.

there was also the same issue re: commented stuff, though I'm not too worried about that

private final Sensor pollTime;

public SourceTaskMetricsGroup(ConnectorTaskId id, ConnectMetrics connectMetrics) {
metricGroup = connectMetrics.group("source-tasks-metrics",
Copy link
Contributor

Choose a reason for hiding this comment

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

from the KIP, I think this is supposed to be source-task-metrics

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ack

@ewencp
Copy link
Contributor

ewencp commented Sep 28, 2017

Ok, based on ack, this LGTM and I'll just clean up that one extra s during merge.

@asfgit asfgit closed this in 89ba0c1 Sep 28, 2017
asfgit pushed a commit that referenced this pull request Oct 3, 2017
Added Connect metrics specific to source tasks, and builds upon #3864 and #3911 that have already been merged into `trunk`, and #3959 that has yet to be merged.

I'll rebase this PR when the latter is merged.

Author: Randall Hauch <rhauch@gmail.com>

Reviewers: Ewen Cheslack-Postava <ewen@confluent.io>

Closes #3975 from rhauch/kafka-5902
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants