Support intra-broker throttling (replica.alter.log.dirs.io.max.bytes.per.second) #2145

dongjinleekr · 2024-04-21T15:14:37Z

This PR resolves #1851.

dongjinleekr · 2024-04-21T15:18:19Z

This PR is an WIP; I will test this feature in our in-house fork (@naver) this week. Stay tuned!! 🙃

dongjinleekr · 2024-06-20T23:58:27Z

@aswinayyolath @mhratson @jiao-zhangS Could you kindly have a look? We adopted this patch to our in-house distribution and confirmed it works correctly. 🙏

…ationThrottledRate

…cationThrottles

- Add: setLogDirThrottles, setLogDirThrottledRateIfNecessary - Rename: removeReplicationThrottledRateFromBroker → removeThrottledRatesFromBroker

…oker rebalanacing - Rename: ReplicationThrottleHelper.clearThrottles → clearInterBrokerThrottles - Add ReplicationThrottleHelper.clearIntraBrokerThrottles - Executor.intraBrokerMoveReplicas now calls ReplicationThrottleHelper.setLogDirThrottles, ReplicationThrottleHelper.clearIntraBrokerThrottles

cpaika

Will make things a lot more stable for JBOD clusters, good change

cpaika · 2024-08-15T15:36:31Z

cruise-control/src/main/java/com/linkedin/kafka/cruisecontrol/executor/Executor.java

@@ -1707,6 +1729,11 @@ private void intraBrokerMoveReplicas() {
        waitForIntraBrokerReplicaTasksToFinish();
        inExecutionTasks = inExecutionTasks();
      }
+
+      if (_logDirThrottle != null) {


for my understanding, why the underscore in the variable names here?

In consistency with _replicationThrottle.

dongjinleekr · 2024-11-17T07:09:58Z

@cpaika @adkafka

Excuse me. Is there any issue with this PR? It initially worked on 2.5.138, and 2.5.141 was already released last month, but this PR has yet to be merged or released.

If any modifications are needed, don't hesitate to leave me a mention.

adkafka · 2024-11-18T16:28:09Z

@cpaika @adkafka

Excuse me. Is there any issue with this PR? It initially worked on 2.5.138, and 2.5.141 was already released last month, but this PR has yet to be merged or released.

If any modifications are needed, don't hesitate to leave me a mention.

I don't see any issues with this PR, but I'm not a maintainer of this project. We need a maintainer to review this and add their approval.

aswinayyolath · 2024-11-18T16:33:01Z

@CCisGG could you please review this PR?

dongjinleekr · 2024-12-07T13:15:12Z

@CCisGG Hello. Could you please have a look when you are free? 🙏

CCisGG · 2024-12-08T19:21:43Z

Hi @mhratson, would you mind taking a look at this one? Thanks!

imans777 · 2025-01-04T14:35:05Z

Anyone caring to review this?
It's really a must-have feature.

CCisGG

Added some comments/questions.

Some other questions I have:

Did you test that with your change, when no replication/intra broker throttling was set (which I think is by default), does this work properly?
Is their any case that this intra-broker throttling can interfere with inter-broker thorttling? E.g. what happens if they are enabled together?
Please update Configuration.md for this new config.

CCisGG · 2025-01-06T17:42:15Z

...-control/src/main/java/com/linkedin/kafka/cruisecontrol/config/constants/ExecutorConfig.java

+   * <code>default.log.dir.throttle</code>
+   */
+  public static final String DEFAULT_LOG_DIR_THROTTLE_CONFIG = "default.log.dir.throttle";
+  public static final Long DEFAULT_DEFAULT_LOG_DIR_THROTTLE = null;


Would that be more reasonable to have it Long.MAX_VALUE rather than null? I'm a bit concerned to have a null default value for a LONG type. I'm not entirely sure what will happen here by default

Nvm. Just realize that default replication throttle is also null. This is fine then.

CCisGG · 2025-01-06T17:44:08Z

cruise-control/src/main/java/com/linkedin/kafka/cruisecontrol/executor/Executor.java

@@ -830,7 +834,7 @@ public synchronized void executeProposals(Collection<ExecutionProposal> proposal
                            requestedIntraBrokerPartitionMovementConcurrency, requestedClusterLeadershipMovementConcurrency,
                            requestedBrokerLeadershipMovementConcurrency, requestedExecutionProgressCheckIntervalMs, replicaMovementStrategy,
                            isTriggeredByUserRequest, loadMonitor);
-      startExecution(loadMonitor, null, removedBrokers, replicationThrottle, isTriggeredByUserRequest);
+      startExecution(loadMonitor, null, removedBrokers, replicationThrottle, logDirThrottle, isTriggeredByUserRequest);


Just curious, does that make more sense to have a separate method for intra-broker throttling since this parameter only applies for inter-broker movement?

CCisGG · 2025-01-06T17:44:49Z

cruise-control/src/main/java/com/linkedin/kafka/cruisecontrol/executor/Executor.java

@@ -918,7 +922,7 @@ public synchronized void executeDemoteProposals(Collection<ExecutionProposal> pr
      initProposalExecution(proposals, demotedBrokers, concurrentSwaps, null, 0,
                            requestedClusterLeadershipMovementConcurrency, requestedBrokerLeadershipMovementConcurrency,
                            requestedExecutionProgressCheckIntervalMs, replicaMovementStrategy, isTriggeredByUserRequest, loadMonitor);
-      startExecution(loadMonitor, demotedBrokers, null, replicationThrottle, isTriggeredByUserRequest);
+      startExecution(loadMonitor, demotedBrokers, null, replicationThrottle, replicationThrottle, isTriggeredByUserRequest);


What is the implication here to have log dir throttling set to replication throttle here?

CCisGG · 2025-01-06T17:45:54Z

...ntrol/src/main/java/com/linkedin/kafka/cruisecontrol/executor/ReplicationThrottleHelper.java

-      for (Map.Entry<String, Set<String>> entry : throttledReplicas.entrySet()) {
-        setThrottledReplicas(entry.getKey(), entry.getValue());
-      }
+    LOG.info("Setting a rebalance throttle of {} bytes/sec", throttleRate);


Why do we remove the "if (throttlingEnabled())" check here?

CCisGG · 2025-01-06T17:47:42Z

...ntrol/src/main/java/com/linkedin/kafka/cruisecontrol/executor/ReplicationThrottleHelper.java

@@ -336,7 +355,7 @@ private void removeThrottledReplicasFromTopic(String topic, Set<String> replicas
    }
  }

-  private void removeThrottledRateFromBroker(Integer brokerId)
+  private void removeThrottledRatesFromBroker(Integer brokerId)


Is this method now become "remove both throttle rates"? In that case we should add a comment to specify that.

dongjinleekr added 6 commits August 15, 2024 16:44

Remove _throttleRate variable from ReplicationThrottleHelper class

a915f7f

Rename ReplicationThrottleHelper method names: ThrottledRate → Replic…

7e324b7

…ationThrottledRate

Rename ReplicationThrottleHelper method name: setThrottles → setRepli…

9c283d7

…cationThrottles

Add ReplicationThrottleHelper#LOG_DIR_THROTTLED_RATE support

4087b85

- Add: setLogDirThrottles, setLogDirThrottledRateIfNecessary - Rename: removeReplicationThrottledRateFromBroker → removeThrottledRatesFromBroker

Add (default.)log.dir.throttle parameter

88aac15

dongjinleekr force-pushed the feature/1851 branch from 4781c13 to c59c864 Compare August 15, 2024 07:44

cpaika approved these changes Aug 15, 2024

View reviewed changes

cpaika mentioned this pull request Sep 13, 2024

Throttling intra broker rebalances #2199

Open

adkafka approved these changes Sep 20, 2024

View reviewed changes

aswinayyolath approved these changes Nov 18, 2024

View reviewed changes

imans777 mentioned this pull request Jan 4, 2025

Rebalance Disks Broker by Broker #2237

Open

CCisGG reviewed Jan 6, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support intra-broker throttling (replica.alter.log.dirs.io.max.bytes.per.second) #2145

Support intra-broker throttling (replica.alter.log.dirs.io.max.bytes.per.second) #2145

dongjinleekr commented Apr 21, 2024

dongjinleekr commented Apr 21, 2024

dongjinleekr commented Jun 20, 2024

cpaika left a comment

cpaika Aug 15, 2024

dongjinleekr Aug 19, 2024

dongjinleekr commented Nov 17, 2024

adkafka commented Nov 18, 2024

aswinayyolath commented Nov 18, 2024

dongjinleekr commented Dec 7, 2024

CCisGG commented Dec 8, 2024

imans777 commented Jan 4, 2025

CCisGG left a comment

CCisGG Jan 6, 2025

CCisGG Jan 6, 2025

CCisGG Jan 6, 2025

CCisGG Jan 6, 2025

CCisGG Jan 6, 2025

CCisGG Jan 6, 2025

Support intra-broker throttling (replica.alter.log.dirs.io.max.bytes.per.second) #2145

Are you sure you want to change the base?

Support intra-broker throttling (replica.alter.log.dirs.io.max.bytes.per.second) #2145

Conversation

dongjinleekr commented Apr 21, 2024

dongjinleekr commented Apr 21, 2024

dongjinleekr commented Jun 20, 2024

cpaika left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dongjinleekr commented Nov 17, 2024

adkafka commented Nov 18, 2024

aswinayyolath commented Nov 18, 2024

dongjinleekr commented Dec 7, 2024

CCisGG commented Dec 8, 2024

imans777 commented Jan 4, 2025

CCisGG 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment