KAFKA-17668: Clean-up LogCleaner#maxOverCleanerThreads and LogCleanerManager#maintainUncleanablePartitions #17390

frankvicky · 2024-10-07T12:51:19Z

Both methods can be rewrite by scala 2.13 methods

Since maxOverCleanerThreads does not have a corresponding unit test, I have added a unit test for it.
maintainUncleanablePartitions has been thoroughly tested in tests, so I simply replaced the old implementation with the new one.

Committer Checklist (excluded from commit message)

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

…anager#maintainUncleanablePartitions

frankvicky · 2024-12-13T15:58:52Z

Hi @chia7712
Could you please take a look ?

ijuma

LGTM, thanks.

ijuma · 2025-01-03T01:33:09Z

Since maxOverCleanerThreads does not have a corresponding unit test, I have added a unit test to ensure the result remains the same after replacing it with the new implementation.

I don't see a unit test in this PR.

frankvicky · 2025-01-03T03:36:01Z

Hi @ijuma,
I previously wrote some unit tests for this PR (see ec74070).

However, I later decided to remove these tests as I realized they might not add much value.
The code being replaced is used by helper methods that are already well covered by existing test cases, which should provide sufficient testing coverage.
WDYT ?

ijuma · 2025-01-15T14:55:51Z

Yes, that's reasonable.

ijuma · 2025-01-15T14:56:14Z

Since maxOverCleanerThreads does not have a corresponding unit test, I have added a unit test to ensure the result remains the same after replacing it with the new implementation.

Please update this description then.

ijuma · 2025-01-15T14:56:43Z

Ah, we do have a new unit test now - I'll review that.

ijuma · 2025-01-15T14:57:36Z

core/src/test/scala/unit/kafka/log/ScalaCompatibilityTest.scala

+import scala.collection.mutable
+
+
+class ScalaCompatibilityTest {


This test name isn't right. This is not about testing scala compatibility, it's about testing the relevant log cleaner method. Is there a test class where this test makes sense? If not, it's ok to add one, but the name should be about the thing being tested.

chia7712

@frankvicky thanks for this patch. Those classes have ut file already, so it would be better to add unit test for the changed methods as there are many existent help methods in the test file.

chia7712 · 2025-01-15T16:07:03Z

core/src/main/scala/kafka/log/LogCleaner.scala

@@ -116,12 +116,11 @@ class LogCleaner(initialConfig: CleanerConfig,
  private[log] val cleaners = mutable.ArrayBuffer[CleanerThread]()

  /**
-   * scala 2.12 does not support maxOption so we handle the empty manually.
   * @param f to compute the result
   * @return the max value (int value) or 0 if there is no cleaner
   */
  private def maxOverCleanerThreads(f: CleanerThread => Double): Int =


Maybe we can add a unit test to LogCleanerTest?

chia7712 · 2025-01-15T16:07:44Z

core/src/main/scala/kafka/log/LogCleanerManager.scala

@@ -533,21 +533,15 @@ private[log] class LogCleanerManager(val logDirs: Seq[File],
  def maintainUncleanablePartitions(): Unit = {


ditto. LogCleanerManagerTest is a good place :)

maintainUncleanablePartitions is invoked by CleanerThread#doWork, which is a daemon thread. It has been ㄎwidely tested through integration tests and unit tests related to LogCleaner. IMHO, we can leverage these existing tests and don't need to write new ones.

ijuma · 2025-01-19T02:18:04Z

core/src/test/scala/unit/kafka/log/LogCleanerTest.scala

+    cleaner3.lastStats.bufferUtilization = 0.65d
+    cleaners += cleaner3
+
+    assertEquals(0, logCleaner.maxOverCleanerThreads(_.lastStats.bufferUtilization))


Hmm, can we also assert a non zero value?

Sure!
I have just added it

In reviewing this test, I noticed that I introduced a bug in #8783 that I round down the double value when updating metrics max-buffer-utilization-percent

I open https://issues.apache.org/jira/browse/KAFKA-18597 to fix it

Good catch. I'll backport this one to 4.0 to make it easier to backport the bug fix.

Also, given the fix, perhaps we don't need the case where we set the bufferUtilization to larger numbers.

Also, given the fix, perhaps we don't need the case where we set the bufferUtilization to larger numbers.

you are right. I copy your comment to #18627

ijuma · 2025-01-19T05:19:52Z

core/src/test/scala/unit/kafka/log/LogCleanerTest.scala

+
+    val cleaner1 = new logCleaner.CleanerThread(1)
+    cleaner1.lastStats = new CleanerStats(time)
+    cleaner1.lastStats.bufferUtilization = 0.75d


Nit: d is not needed, 0.75 is already a double. Same for a few more cases below.

Thank you 😸
I have removed the redundant d but still keep it for the integer.

ijuma

LGTM, thanks.

…Manager#maintainUncleanablePartitions (#17390) Since maxOverCleanerThreads does not have a corresponding unit test, I have added a unit test for it. maintainUncleanablePartitions has been thoroughly tested in tests, so I simply replaced the old implementation with the new one. Reviewers: Ismael Juma <[email protected]>, Chia-Ping Tsai <[email protected]>

…Manager#maintainUncleanablePartitions (apache#17390) Since maxOverCleanerThreads does not have a corresponding unit test, I have added a unit test for it. maintainUncleanablePartitions has been thoroughly tested in tests, so I simply replaced the old implementation with the new one. Reviewers: Ismael Juma <[email protected]>, Chia-Ping Tsai <[email protected]>

KAFKA-17668: Rewrite LogCleaner#maxOverCleanerThreads and LogCleanerM…

a85b26f

…anager#maintainUncleanablePartitions

github-actions bot added core Kafka Broker small Small PRs labels Oct 7, 2024

frankvicky added 2 commits October 8, 2024 10:42

Merge branch 'trunk' into KAFKA-17668

1839a06

Merge branch 'trunk' into KAFKA-17668

6dbd244

frankvicky added the ci-approved label Oct 26, 2024

frankvicky marked this pull request as draft November 14, 2024 14:23

frankvicky marked this pull request as ready for review November 14, 2024 14:23

KAFKA-17668: revert unnecessary change

ec74070

ijuma approved these changes Jan 3, 2025

View reviewed changes

Merge branch 'trunk' into KAFKA-17668

b835c1f

frankvicky force-pushed the KAFKA-17668 branch from 58513e9 to b835c1f Compare January 3, 2025 03:11

KAFKA-17668: Add unit test back.

a9468ec

Merge branch 'trunk' into KAFKA-17668

ffe6689

ijuma reviewed Jan 15, 2025

View reviewed changes

chia7712 reviewed Jan 15, 2025

View reviewed changes

KAFKA-17668: Move test to LogCleanerTest

38ecab8

github-actions bot removed the small Small PRs label Jan 16, 2025

Merge branch 'trunk' into KAFKA-17668

e2ad6f6

frankvicky force-pushed the KAFKA-17668 branch from e93a377 to e2ad6f6 Compare January 16, 2025 06:13

github-actions bot added the small Small PRs label Jan 16, 2025

frankvicky added 2 commits January 17, 2025 16:16

Merge branch 'trunk' into KAFKA-17668

4530b27

Merge branch 'trunk' into KAFKA-17668

f0370c0

ijuma reviewed Jan 19, 2025

View reviewed changes

frankvicky added 3 commits January 19, 2025 13:21

KAFKA-17668: Add more test case.

62483f1

Merge branch 'trunk' into KAFKA-17668

80cbd3d

Merge branch 'trunk' into KAFKA-17668

83d61d2

frankvicky force-pushed the KAFKA-17668 branch from 8283d31 to 83d61d2 Compare January 19, 2025 05:22

ijuma approved these changes Jan 19, 2025

View reviewed changes

chia7712 approved these changes Jan 19, 2025

View reviewed changes

ijuma merged commit 697cf64 into apache:trunk Jan 19, 2025
9 checks passed

ijuma changed the title ~~KAFKA-17668: Rewrite LogCleaner#maxOverCleanerThreads and LogCleanerManager#maintainUncleanablePartitions~~ KAFKA-17668: Clean-up LogCleaner#maxOverCleanerThreads and LogCleanerManager#maintainUncleanablePartitions Jan 19, 2025

chia7712 mentioned this pull request Jan 19, 2025

KAFKA-18597 Fix max-buffer-utilization-percent is always 0 #18627

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

KAFKA-17668: Clean-up LogCleaner#maxOverCleanerThreads and LogCleanerManager#maintainUncleanablePartitions #17390

KAFKA-17668: Clean-up LogCleaner#maxOverCleanerThreads and LogCleanerManager#maintainUncleanablePartitions #17390

frankvicky commented Oct 7, 2024 •

edited

Loading

frankvicky commented Dec 13, 2024

ijuma left a comment

ijuma commented Jan 3, 2025

frankvicky commented Jan 3, 2025

ijuma commented Jan 15, 2025

ijuma commented Jan 15, 2025

ijuma commented Jan 15, 2025

ijuma Jan 15, 2025

chia7712 left a comment

chia7712 Jan 15, 2025

chia7712 Jan 15, 2025

frankvicky Jan 16, 2025

ijuma Jan 19, 2025

frankvicky Jan 19, 2025 •

edited

Loading

chia7712 Jan 19, 2025

ijuma Jan 19, 2025

ijuma Jan 19, 2025

chia7712 Jan 19, 2025

ijuma Jan 19, 2025 •

edited

Loading

frankvicky Jan 19, 2025

ijuma left a comment

		import scala.collection.mutable


		class ScalaCompatibilityTest {

		@@ -533,21 +533,15 @@ private[log] class LogCleanerManager(val logDirs: Seq[File],
		def maintainUncleanablePartitions(): Unit = {

KAFKA-17668: Clean-up LogCleaner#maxOverCleanerThreads and LogCleanerManager#maintainUncleanablePartitions #17390

KAFKA-17668: Clean-up LogCleaner#maxOverCleanerThreads and LogCleanerManager#maintainUncleanablePartitions #17390

Conversation

frankvicky commented Oct 7, 2024 • edited Loading

Committer Checklist (excluded from commit message)

frankvicky commented Dec 13, 2024

ijuma left a comment

Choose a reason for hiding this comment

ijuma commented Jan 3, 2025

frankvicky commented Jan 3, 2025

ijuma commented Jan 15, 2025

ijuma commented Jan 15, 2025

ijuma commented Jan 15, 2025

Choose a reason for hiding this comment

chia7712 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

frankvicky Jan 19, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ijuma Jan 19, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ijuma left a comment

Choose a reason for hiding this comment

frankvicky commented Oct 7, 2024 •

edited

Loading

frankvicky Jan 19, 2025 •

edited

Loading

ijuma Jan 19, 2025 •

edited

Loading