Skip to content

KAFKA-12519: Remove built-in Streams metrics for versions 0.10.0-2.4#10765

Merged
cadonna merged 3 commits into
apache:trunkfrom
cadonna:AK12519-remove_old_builtin_metrics
Jun 1, 2021
Merged

KAFKA-12519: Remove built-in Streams metrics for versions 0.10.0-2.4#10765
cadonna merged 3 commits into
apache:trunkfrom
cadonna:AK12519-remove_old_builtin_metrics

Conversation

@cadonna

@cadonna cadonna commented May 26, 2021

Copy link
Copy Markdown
Member

As specified in KIP-743, this PR removes the built-in metrics
in Streams that are superseded by the refactoring proposed in KIP-444.

Committer Checklist (excluded from commit message)

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

@cadonna

cadonna commented May 26, 2021

Copy link
Copy Markdown
Member Author

Sorry for this massive cleanup PR: @guozhangwang @mjsax @ableegoldman @vvcephei @abbccdda

showuon
showuon reviewed May 26, 2021
View reviewed changes

@showuon showuon 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.

@cadonna , thanks for the PR. I have a quick look, left some comments. In additional to that, I did a search, and found there are still 2 places with related variables that should be handled.

  1. THREAD_ID_TAG_0100_TO_24 variable in StreamsMetricsImpl.java
  2. THREAD_ID_TAG_0100_TO_24 variable in StreamTaskTest.java

Thank you.

Comment thread ...s/src/main/java/org/apache/kafka/streams/processor/internals/metrics/StreamsMetricsImpl.java Outdated

@showuon showuon May 26, 2021

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.

After removing FROM_0100_TO_24, I think we can remove this parseBuiltInMetricsVersion, too.

@cadonna cadonna May 26, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Removed it.

Comment thread ...s/src/main/java/org/apache/kafka/streams/processor/internals/metrics/StreamsMetricsImpl.java Outdated

@showuon showuon May 26, 2021

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.

If the enum only has 1 element and named as LATEST, did we still need this enum?

@cadonna cadonna May 26, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Since we decided to keep the config built.in.metrics.version I think it is OK to keep also this enum.

@cadonna

cadonna commented May 26, 2021

Copy link
Copy Markdown
Member Author

I did a search, and found there are still 2 places with related variables that should be handled.

@showuon Nice catch!

guozhangwang
guozhangwang approved these changes May 26, 2021
View reviewed changes

@guozhangwang guozhangwang left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for the effort! Made a pass. Did not spot anything yellow.

Comment thread streams/src/test/java/org/apache/kafka/streams/processor/internals/metrics/TaskMetricsTest.java Outdated

@guozhangwang guozhangwang May 26, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Learned something new about andStubReturn v.s. andReturn :)

Comment thread streams/src/test/java/org/apache/kafka/streams/state/internals/MeteredWindowStoreTest.java Outdated

@guozhangwang guozhangwang May 26, 2021

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

We forgot to add this before?

@cadonna cadonna May 27, 2021

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Good that you mention that! I checked the metered state store tests and found some testing holes.

@cadonna cadonna force-pushed the AK12519-remove_old_builtin_metrics branch from 8a8c97d to 2afe7b3 Compare May 31, 2021 10:01
cadonna added 3 commits May 31, 2021 14:09
@cadonna
KAFKA-12519: Remove built-in Streams metrics for versions 0.10.0-2.4
fe929a4 
As specified in KIP-743, this PR removes the built-in metrics
in Streams that are superseded by the refactoring proposed in KIP-444.
@cadonna
Remove missed variables to remove
8444f7e
@cadonna
Fix unit tests for metered stores
cb253df
@cadonna cadonna force-pushed the AK12519-remove_old_builtin_metrics branch from 2afe7b3 to cb253df Compare May 31, 2021 12:09
@cadonna

cadonna commented May 31, 2021

Copy link
Copy Markdown
Member Author

@showuon and @guozhangwang Do you want to have a second look after my last update of the unit tests? Otherwise I would move on and merge.

@cadonna

cadonna commented May 31, 2021

Copy link
Copy Markdown
Member Author

Test failures are unrelated and known to be flaky:

JDK 8 and Scala 2.12 / kafka.server.RaftClusterTest.testCreateClusterAndCreateAndManyTopics()
JDK 8 and Scala 2.12 / kafka.server.RaftClusterTest.testCreateClusterAndCreateAndManyTopics()
JDK 15 and Scala 2.13 / kafka.server.RaftClusterTest.testCreateClusterAndCreateAndManyTopicsWithManyPartitions()
JDK 15 and Scala 2.13 / kafka.server.RaftClusterTest.testCreateClusterAndCreateListDeleteTopic()

showuon
showuon approved these changes Jun 1, 2021
View reviewed changes

@showuon showuon 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! Thanks for the PR!

@cadonna

cadonna commented Jun 1, 2021

Copy link
Copy Markdown
Member Author

@showuon Thanks for the review!

@cadonna cadonna merged commit cfe642e into apache:trunk Jun 1, 2021
guozhangwang
guozhangwang reviewed Jun 1, 2021
View reviewed changes

@guozhangwang guozhangwang left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Made a quick pass on the latest two commits, LGTM.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@guozhangwang guozhangwang guozhangwang approved these changes

@showuon showuon showuon approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@cadonna @guozhangwang @showuon