KAFKA-9066: Retain metrics for failed tasks by C0urante · Pull Request #8502 · apache/kafka

C0urante · 2020-04-16T20:25:29Z

Right now, sink and source task JMX metrics are dropped as soon as the task fails. The changes here cause these metrics to be retained even if the task fails, and instead only be removed when the task has been shut down (in preparation for a rebalance, due to reconfiguration, or because of connector deletion) or abandoned during that shutdown process by the worker.

Existing unit tests are modified to account for this tweak in the worker logic.

Committer Checklist (excluded from commit message)

Verify design and implementation
Verify test coverage and CI build status
Verify documentation (including upgrade notes)

C0urante · 2020-04-16T20:25:58Z

@gharris1727, @cyrusv, @ncliang, would any of you mind taking a look when you have a moment?

ncliang

Thanks @C0urante . I have just one question about the need for separate methods to be implemented instead of overriding the method in subclass.

ncliang

Thanks @C0urante . LGTM.

C0urante · 2020-04-18T00:15:31Z

Thanks @ncliang! @rhauch, @kkonstantine would one of you mind taking a look at this when you have a moment?

rhauch · 2020-05-23T14:04:05Z

ok to test

rhauch · 2020-06-05T19:09:44Z

retest this please

rhauch · 2020-06-10T02:46:36Z

Retest this please

rhauch · 2020-06-11T00:43:56Z

One concern I have is that we're not actually verifying in any of the tests that the task metrics are cleaned up when a connector is deleted, and that the tasks that remain after a worker fails are cleared and don't hinder the re-creation of the task metrics if the connector is restarted. Thoughts?

C0urante · 2020-06-11T01:51:16Z

@rhauch it'd be nice to verify this in tests but unfortunately I didn't notice your comment earlier in the day and I don't think I have time to adjust/write new test cases to account for this. It also looks like the possibility you pointed out about hindering re-creation of task metrics isn't super likely given that tasks proactively clear out any metrics set up for older instances on startup:

kafka/connect/runtime/src/main/java/org/apache/kafka/connect/runtime/WorkerTask.java

Lines 352 to 353 in 6abb913

    
           // prevent collisions by removing any previously created metrics in this group. 
        
           metricGroup.close();

If this is still a sticking point, could we log a backlog ticket for those testing improvements and address it as time permits after the 2.6 release?

rhauch · 2020-06-11T02:03:22Z

@C0urante the original PR description doesn't mention that the task metrics for a failed task will also be removed when the connector is deleted (in addition to the worker stopping or the tasks completing gracefully). Can you confirm my understanding, and if I'm right can you please update the description?

C0urante · 2020-06-11T02:07:44Z

@rhauch sure, I can update the description to make it clear that we will still remove task metrics when a connector is deleted.

rhauch

Thanks, @C0urante. LGTM.

Author: Chris Egerton <chrise@confluent.io> Reviewers: Nigel Liang <nigel@nigelliang.com>, Randall Hauch <rhauch@gmail.com>

* 'trunk' of github.com:apache/kafka: (42 commits) HOTFIX: Fix compile error in TopicAdminTest (apache#8866) KAFKA-10144: clean up corrupted standby tasks before attempting a commit (apache#8849) KAFKA-10157: Fix broken tests due to InterruptedException from FinalizedFeatureChangeListener (apache#8857) KAFKA-9432: automated protocol for DescribeConfigs (apache#8312) KAFKA-10049: Fixed FKJ bug where wrapped serdes are set incorrectly when using default StreamsConfig serdes (apache#8764) KAFKA-10027: Implement read path for feature versioning system (KIP-584) (apache#8680) KAFKA-10085: correctly compute lag for optimized source changelogs (apache#8787) KAFKA-10086: Integration test for ensuring warmups are effective (apache#8818) KAFKA-9374: Make connector interactions asynchronous (apache#8069) MINOR: reduce sizeInBytes for percentiles metrics (apache#8835) KAFKA-10115: Incorporate errors.tolerance with the Errant Record Reporter (apache#8829) KAFKA-9216: Enforce that Connect’s internal topics use `compact` cleanup policy (apache#8828) KAFKA-9845: Warn users about using config providers with plugin.path property (apache#8455) KAFKA-7833: Add missing test (apache#8847) KAFKA-9066: Retain metrics for failed tasks (apache#8502) KAFKA-9841: Revoke duplicate connectors and tasks when zombie workers return with an outdated assignment (apache#8453) KAFKA-9985: Sink connector may exhaust broker when writing in DLQ (apache#8663) KAFKA-9441: remove prepareClose() to simplify task management (apache#8833) KAFKA-7833: Add Global/StateStore name conflict check (apache#8825) KAFKA-9969: Exclude ConnectorClientConfigRequest from class loading isolation (apache#8630) ...

Author: Chris Egerton <chrise@confluent.io> Reviewers: Nigel Liang <nigel@nigelliang.com>, Randall Hauch <rhauch@gmail.com>

Author: Chris Egerton <chrise@confluent.io> Reviewers: Nigel Liang <nigel@nigelliang.com>, Randall Hauch <rhauch@gmail.com>, Konstantine Karantasis <konstantine@confluent.io>

Author: Chris Egerton <chrise@confluent.io> Reviewers: Nigel Liang <nigel@nigelliang.com>, Konstantine Karantasis <konstantine@confluent.io>, Randall Hauch <rhauch@gmail.com>

…t-for-generated-requests * apache-github/trunk: (248 commits) KAFKA-10049: Fixed FKJ bug where wrapped serdes are set incorrectly when using default StreamsConfig serdes (apache#8764) KAFKA-10027: Implement read path for feature versioning system (KIP-584) (apache#8680) KAFKA-10085: correctly compute lag for optimized source changelogs (apache#8787) KAFKA-10086: Integration test for ensuring warmups are effective (apache#8818) KAFKA-9374: Make connector interactions asynchronous (apache#8069) MINOR: reduce sizeInBytes for percentiles metrics (apache#8835) KAFKA-10115: Incorporate errors.tolerance with the Errant Record Reporter (apache#8829) KAFKA-9216: Enforce that Connect’s internal topics use `compact` cleanup policy (apache#8828) KAFKA-9845: Warn users about using config providers with plugin.path property (apache#8455) KAFKA-7833: Add missing test (apache#8847) KAFKA-9066: Retain metrics for failed tasks (apache#8502) KAFKA-9841: Revoke duplicate connectors and tasks when zombie workers return with an outdated assignment (apache#8453) KAFKA-9985: Sink connector may exhaust broker when writing in DLQ (apache#8663) KAFKA-9441: remove prepareClose() to simplify task management (apache#8833) KAFKA-7833: Add Global/StateStore name conflict check (apache#8825) KAFKA-9969: Exclude ConnectorClientConfigRequest from class loading isolation (apache#8630) KAFKA-9991: Fix flaky unit tests (apache#8843) KAFKA-10014; Always try to close all channels in Selector#close (apache#8685) KAFKA-10079: improve thread-level stickiness (apache#8775) MINOR: Print all removed dynamic members during join complete (apache#8816) ...

ncliang reviewed Apr 17, 2020

View reviewed changes

Comment thread connect/runtime/src/main/java/org/apache/kafka/connect/runtime/WorkerTask.java Outdated

ncliang approved these changes Apr 17, 2020

View reviewed changes

kkonstantine added the connect label Apr 30, 2020

strigona-worksight mentioned this pull request Apr 30, 2020

Kafka Connect JMX : source & sink task metrics missing for tasks in failed state confluentinc/cp-docker-images#808

Closed

C0urante added 3 commits May 29, 2020 12:03

KAFKA-9066: Retain metrics for failed tasks

b7e4f19

KAFKA-9066: Address review feedback

d191a22

KAFKA-9066: Fix unit tests

2300698

C0urante force-pushed the kafka-9066 branch from f24f682 to 2300698 Compare May 29, 2020 19:23

rhauch approved these changes Jun 11, 2020

View reviewed changes

rhauch merged commit 775f0d4 into apache:trunk Jun 11, 2020

rhauch pushed a commit that referenced this pull request Jun 11, 2020

KAFKA-9066: Retain metrics for failed tasks (#8502)

eb58b61

Author: Chris Egerton <chrise@confluent.io> Reviewers: Nigel Liang <nigel@nigelliang.com>, Randall Hauch <rhauch@gmail.com>

rhauch pushed a commit to rhauch/kafka that referenced this pull request Jun 11, 2020

KAFKA-9066: Retain metrics for failed tasks (apache#8502)

1f4c379

Author: Chris Egerton <chrise@confluent.io> Reviewers: Nigel Liang <nigel@nigelliang.com>, Randall Hauch <rhauch@gmail.com>

rhauch mentioned this pull request Jun 11, 2020

KAFKA-10146, KAFKA-9066: Retain metrics for failed tasks (backport to 2.5) #8854

Merged

3 tasks

rhauch pushed a commit to rhauch/kafka that referenced this pull request Jul 30, 2020

KAFKA-9066: Retain metrics for failed tasks (apache#8502)

127bcf4

Author: Chris Egerton <chrise@confluent.io> Reviewers: Nigel Liang <nigel@nigelliang.com>, Randall Hauch <rhauch@gmail.com>

rhauch mentioned this pull request Jul 30, 2020

KAFKA-10146, KAFKA-9066: Retain metrics for failed tasks (backport to 2.4) #9106

Merged

3 tasks

C0urante deleted the kafka-9066 branch November 19, 2021 14:20

Uh oh!

Conversation

C0urante commented Apr 16, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Committer Checklist (excluded from commit message)

Uh oh!

C0urante commented Apr 16, 2020

Uh oh!

ncliang left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ncliang left a comment

Choose a reason for hiding this comment

Uh oh!

C0urante commented Apr 18, 2020

Uh oh!

rhauch commented May 23, 2020

Uh oh!

rhauch commented Jun 5, 2020

Uh oh!

rhauch commented Jun 10, 2020

Uh oh!

rhauch commented Jun 11, 2020

Uh oh!

C0urante commented Jun 11, 2020

Uh oh!

rhauch commented Jun 11, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

C0urante commented Jun 11, 2020

Uh oh!

rhauch left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

C0urante commented Apr 16, 2020 •

edited

Loading

rhauch commented Jun 11, 2020 •

edited

Loading