Skip to content

KAFKA-10199: Add task updater metrics, part 1#13228

Merged
guozhangwang merged 6 commits into
apache:trunkfrom
guozhangwang:K10199-task-updater-metrics-p1
Feb 24, 2023
Merged

KAFKA-10199: Add task updater metrics, part 1#13228
guozhangwang merged 6 commits into
apache:trunkfrom
guozhangwang:K10199-task-updater-metrics-p1

Conversation

@guozhangwang

@guozhangwang guozhangwang commented Feb 10, 2023

Copy link
Copy Markdown
Contributor
  1. Moved pausing-tasks logic out of the commit-interval loop to be on the top-level loop, similar to resuming tasks.
  2. Added thread-level restoration metrics.
  3. Related unit tests.

Should only be reviewed after #13025

Committer Checklist (excluded from commit message)

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

@guozhangwang

Copy link
Copy Markdown
Contributor Author

cc @lucasbru , would update after #13025 is done, and the pausing logic is extracted out of the checkAllUpdatingTaskStates function.

@guozhangwang
guozhangwang force-pushed the K10199-task-updater-metrics-p1 branch 2 times, most recently from d33b743 to 20e5a4d Compare February 21, 2023 23:19
@guozhangwang guozhangwang changed the title [DRAFT] KAFKA-10199: Add task updater metrics, part 1 KAFKA-10199: Add task updater metrics, part 1 Feb 21, 2023

@guozhangwang guozhangwang left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

ping @lucasbru @cadonna for reviews.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is to avoid checkstyle rules on func complexity, without much logical change (except letting restoreChangelog to return restored records count).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

This is related to 0) in PR description.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Some minor logging fixes piggy-backed here.

@lucasbru lucasbru left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

lgtm, approving. Added minor comments, update as you prefer

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not sure, but is there any performance concern around running this loop in every single iteration of runOnce ?

@guozhangwang guozhangwang Feb 22, 2023

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I think the perf impact should be small since pause/resume are not commonly used, and if the named topology are not paused, then checking the status is just a few cpu cycles.

Another motivation is that if we remove named topologies, than pausing / resuming would always be impact on all tasks, in which case we could have a simpler check (e.g. just check a single flag) which would be even cheaper. So it's probably better to maintain the code layout in this way.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Update the log message as well. This function isn't really checking task states anymore

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

As I understand it, this function will be called quite frequently to export metrics. We only need the size of the collection. It could make sense to avoid the allocations here and just implement a getNumberOfUpdaingActiveTasks as a non-essential but free optimization. Similar for getPausedStandbyTasks etc. pp.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That's a good point, will update.

@guozhangwang
guozhangwang force-pushed the K10199-task-updater-metrics-p1 branch from 8515538 to 57c7ef8 Compare February 22, 2023 17:28
@mjsax mjsax added the streams label Feb 24, 2023
@guozhangwang
guozhangwang merged commit 2fad165 into apache:trunk Feb 24, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants