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-15176: add tests for tiered storage metrics #13999

Merged
merged 11 commits into from Jul 21, 2023
Merged

KAFKA-15176: add tests for tiered storage metrics #13999

merged 11 commits into from Jul 21, 2023

Conversation

showuon
Copy link
Contributor

@showuon showuon commented Jul 12, 2023

Added tests for metrics:

  1. RemoteLogReaderTaskQueueSize
  2. RemoteLogReaderAvgIdlePercent
  3. RemoteLogManagerTasksAvgIdlePercent

Also, added tests for OffsetOutOfRangeException will be thrown while reading logs

Committer Checklist (excluded from commit message)

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

@divijvaidya divijvaidya added the tiered-storage Pull requests associated with KIP-405 (Tiered Storage) label Jul 12, 2023
@satishd satishd self-requested a review July 13, 2023 08:42
clolov
clolov reviewed Jul 18, 2023
View reviewed changes
Copy link
Collaborator

@clolov clolov left a comment

Choose a reason for hiding this comment

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

I started on my first pass and left a couple of comments. I will circle back later on.

core/src/main/java/kafka/log/remote/RemoteLogManager.java Outdated Show resolved Hide resolved
core/src/main/java/kafka/log/remote/RemoteLogReader.java Outdated Show resolved Hide resolved
@showuon showuon marked this pull request as ready for review July 19, 2023 07:28
@showuon
Copy link
Contributor Author

showuon commented Jul 19, 2023

This PR is ready for review now.

clolov
clolov approved these changes Jul 19, 2023
View reviewed changes
Copy link
Collaborator

@clolov clolov left a comment

Choose a reason for hiding this comment

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

Thank you! It looks good to me barring the two small comments.

core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala Outdated
// RemoteLogReader should not be all idle
assertTrue(yammerMetricValue("RemoteLogReaderAvgIdlePercent").asInstanceOf[Double] < 1.0)
// RemoteLogReader should queue some tasks
assertTrue(yammerMetricValue("RemoteLogReaderTaskQueueSize").asInstanceOf[Int] > 0)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I assume the above cannot be equal to 0.0 because of some uncertainty tolerance of when the metric is recorded, but why can this not be equal to 3?

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 question, @clolov !
For RemoteLogReaderAvgIdlePercent, it should be 0.0 since all threads are blocked by the latch.
For RemoteLogReaderTaskQueueSize, yes, it should be 3.

I verified them as a conservative way to tolerate some uncertainties. But maybe I can assert the 2nd one to be 3. If it becomes flaky, we can relieve the assertion.

core/src/test/java/kafka/log/remote/RemoteLogManagerTest.java Show resolved Hide resolved
clolov
clolov approved these changes Jul 20, 2023
View reviewed changes
Copy link
Collaborator

@clolov clolov left a comment

Choose a reason for hiding this comment

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

Great! Thank you for the explanation and the change, the latest version looks ready for merging to me!

satishd
satishd approved these changes Jul 20, 2023
View reviewed changes
Copy link
Member

@satishd satishd left a comment

Choose a reason for hiding this comment

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

Thanks @showuon for the PR. LGTM

@showuon showuon merged commit 27ea025 into apache:trunk Jul 21, 2023
1 check failed
@satishd
Copy link
Member

satishd commented Jul 21, 2023 •
edited

It seems this commit caused the below build failure. I raised #14065 to fix it.

[Error] /Users/satishd/repos/apache-kafka/core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala:3526:77: the result type of an implicit conversion must be more specific than Object
[Error] /Users/satishd/repos/apache-kafka/core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala:3530:70: the result type of an implicit conversion must be more specific than Object
[Warn] /Users/satishd/repos/apache-kafka/core/src/test/scala/unit/kafka/server/ServerGenerateBrokerIdTest.scala:23:21: imported `QuorumTestHarness` is permanently hidden by definition of object QuorumTestHarness in package server
[Warn] /Users/satishd/repos/apache-kafka/core/src/test/scala/unit/kafka/server/ServerGenerateClusterIdTest.scala:29:21: imported `QuorumTestHarness` is permanently hidden by definition of object QuorumTestHarness in package server
[Error] /Users/satishd/repos/apache-kafka/core/src/test/scala/unit/kafka/utils/TestUtils.scala:1438:15: ambiguous reference to overloaded definition,
both method doReturn in class Mockito of type (x$1: Any, x$2: Object*)org.mockito.stubbing.Stubber
and  method doReturn in class Mockito of type (x$1: Any)org.mockito.stubbing.Stubber
match argument types (kafka.log.UnifiedLog)

jeqo pushed a commit to jeqo/kafka that referenced this pull request Jul 21, 2023
@showuon @jeqo
KAFKA-15176: add tests for tiered storage metrics (apache#13999)
0921657 
Added tests for metrics:
1. RemoteLogReaderTaskQueueSize
2. RemoteLogReaderAvgIdlePercent
3. RemoteLogManagerTasksAvgIdlePercent

Also, added tests for OffsetOutOfRangeException will be thrown while reading logs

Reviewers: Christo Lolov <christololov@gmail.com>, Satish Duggana <satishd@apache.org>
Cerchie pushed a commit to Cerchie/kafka that referenced this pull request Jul 25, 2023
@showuon
KAFKA-15176: add tests for tiered storage metrics (apache#13999)
f01d213 
Added tests for metrics:
1. RemoteLogReaderTaskQueueSize
2. RemoteLogReaderAvgIdlePercent
3. RemoteLogManagerTasksAvgIdlePercent

Also, added tests for OffsetOutOfRangeException will be thrown while reading logs

Reviewers: Christo Lolov <christololov@gmail.com>, Satish Duggana <satishd@apache.org>
jeqo pushed a commit to aiven/kafka that referenced this pull request Aug 15, 2023
@showuon @jeqo
KAFKA-15176: add tests for tiered storage metrics (apache#13999)
2126101 
Added tests for metrics:
1. RemoteLogReaderTaskQueueSize
2. RemoteLogReaderAvgIdlePercent
3. RemoteLogManagerTasksAvgIdlePercent

Also, added tests for OffsetOutOfRangeException will be thrown while reading logs

Reviewers: Christo Lolov <christololov@gmail.com>, Satish Duggana <satishd@apache.org>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@satishd satishd satishd approved these changes

@clolov clolov clolov approved these changes

Assignees
No one assigned
Labels
tiered-storage Pull requests associated with KIP-405 (Tiered Storage)
Projects
None yet
Milestone
No milestone
5 participants
@showuon @satishd @clolov @divijvaidya @abhijeetk88