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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 3 commits into from
Jun 1, 2021
Merged

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

merged 3 commits into from
Jun 1, 2021

Conversation

cadonna
Copy link
Contributor

@cadonna cadonna commented May 26, 2021

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
Copy link
Contributor Author

cadonna commented May 26, 2021

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

showuon
showuon reviewed May 26, 2021
View reviewed changes
Copy link
Contributor

@showuon showuon left a comment

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.

...s/src/main/java/org/apache/kafka/streams/processor/internals/metrics/StreamsMetricsImpl.java Outdated
@@ -175,11 +171,7 @@ public StreamsMetricsImpl(final Metrics metrics,
}

private static Version parseBuiltInMetricsVersion(final String builtInMetricsVersion) {
Copy link
Contributor

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed it.

...s/src/main/java/org/apache/kafka/streams/processor/internals/metrics/StreamsMetricsImpl.java
@@ -52,8 +51,7 @@
public class StreamsMetricsImpl implements StreamsMetrics {

public enum Version {
LATEST,
FROM_0100_TO_24
LATEST
Copy link
Contributor

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?

Copy link
Contributor 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
Copy link
Contributor Author

cadonna commented May 26, 2021

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
Copy link
Contributor

@guozhangwang guozhangwang left a comment

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.

streams/src/test/java/org/apache/kafka/streams/processor/internals/metrics/TaskMetricsTest.java
@Before
public void setUp() {
expect(streamsMetrics.version()).andReturn(builtInMetricsVersion).anyTimes();
expect(streamsMetrics.version()).andStubReturn(Version.LATEST);
Copy link
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 :)

streams/src/test/java/org/apache/kafka/streams/state/internals/MeteredWindowStoreTest.java
).metricValue());
}

verify(innerStoreMock);
Copy link
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?

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 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
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
Copy link
Contributor Author

cadonna commented May 31, 2021

@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
Copy link
Contributor Author

cadonna commented May 31, 2021

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
Copy link
Contributor

@showuon showuon left a comment

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
Copy link
Contributor Author

cadonna commented Jun 1, 2021

@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
Copy link
Contributor

@guozhangwang guozhangwang left a comment

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