Skip to content

Rate limit write load decider logging #3

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
+29 −6
Open

Rate limit write load decider logging #3

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
10 changes: 10 additions & 0 deletions ...c/main/java/org/elasticsearch/cluster/routing/allocation/WriteLoadConstraintSettings.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,16 @@ public boolean disabled() {
Setting.Property.NodeScope
);

/**
* The minimum amount of time between logging messages about write load decider interventions
*/
public static final Setting<TimeValue> WRITE_LOAD_DECIDER_MINIMUM_LOGGING_INTERVAL = Setting.timeSetting(
SETTING_PREFIX + "log_interval",
TimeValue.timeValueMinutes(1),
Setting.Property.Dynamic,
Setting.Property.NodeScope
);
Comment on lines +112 to +120
Copy link

Choose a reason for hiding this comment

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

Suggestion: Ensure the class API surface includes access to the new setting by exposing getMinimumLoggingInterval() and make the stored value updateable by adding an initializeAndWatch call in the constructor to update minimumLoggingInterval. [maintainability]


private volatile WriteLoadDeciderStatus writeLoadDeciderStatus;
private volatile TimeValue minimumRerouteInterval;
private volatile double highUtilizationThreshold;
Expand Down
22 changes: 17 additions & 5 deletions ...java/org/elasticsearch/cluster/routing/allocation/decider/WriteLoadConstraintDecider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,8 +18,10 @@
import org.elasticsearch.cluster.routing.ShardRouting;
import org.elasticsearch.cluster.routing.allocation.RoutingAllocation;
import org.elasticsearch.cluster.routing.allocation.WriteLoadConstraintSettings;
import org.elasticsearch.common.FrequencyCappedAction;
import org.elasticsearch.common.settings.ClusterSettings;
import org.elasticsearch.core.Strings;
import org.elasticsearch.core.TimeValue;
import org.elasticsearch.threadpool.ThreadPool;
Copy link

Choose a reason for hiding this comment

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

ThreadPool Injection Opportunity

ThreadPool is imported but not used in constructor while System::currentTimeMillis is used directly. ThreadPool.relativeTimeInMillis() would provide consistent time source and better testability.

Standards
  • SOLID Dependency Inversion
  • Clean Code Consistency


/**
Expand All @@ -31,10 +33,16 @@ public class WriteLoadConstraintDecider extends AllocationDecider {

public static final String NAME = "write_load";

private final FrequencyCappedAction logInterventionMessage;
private final WriteLoadConstraintSettings writeLoadConstraintSettings;

public WriteLoadConstraintDecider(ClusterSettings clusterSettings) {
this.writeLoadConstraintSettings = new WriteLoadConstraintSettings(clusterSettings);
logInterventionMessage = new FrequencyCappedAction(System::currentTimeMillis, TimeValue.ZERO);
Copy link

Choose a reason for hiding this comment

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

System Clock Dependency

System::currentTimeMillis creates direct dependency on system clock which can cause issues during clock adjustments or in testing environments. Clock changes can break rate limiting behavior and make testing non-deterministic.

Standards
  • ISO-25010 Time Behaviour
  • Clean Code Testability

clusterSettings.initializeAndWatch(
WriteLoadConstraintSettings.WRITE_LOAD_DECIDER_MINIMUM_LOGGING_INTERVAL,
logInterventionMessage::setMinInterval
);
}

@Override
Expand Down Expand Up @@ -77,7 +85,9 @@ public Decision canAllocate(ShardRouting shardRouting, RoutingNode node, Routing
nodeWriteThreadPoolLoadThreshold,
shardRouting.shardId()
);
logger.debug(explain);
if (logger.isDebugEnabled()) {
logInterventionMessage.maybeExecute(() -> logger.debug(explain));
}
Comment on lines +88 to +90
Copy link

Choose a reason for hiding this comment

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

medium

This rate-limited logging logic is duplicated in three places within this class (here, lines 110-112, and 160-162). To improve maintainability and reduce code duplication, consider extracting this logic into a private helper method.

For example, you could add:

private void maybeLogIntervention(String explanation) {
    if (logger.isDebugEnabled()) {
        logInterventionMessage.maybeExecute(() -> logger.debug(explanation));
    }
}

And then replace the duplicated blocks with a call to maybeLogIntervention(explain);.

return Decision.single(Decision.Type.NOT_PREFERRED, NAME, explain);
}

Expand All @@ -97,7 +107,9 @@ public Decision canAllocate(ShardRouting shardRouting, RoutingNode node, Routing
shardWriteLoad,
nodeWriteThreadPoolStats.totalThreadPoolThreads()
);
logger.debug(explain);
if (logger.isDebugEnabled()) {
logInterventionMessage.maybeExecute(() -> logger.debug(explain));
}
return Decision.single(Decision.Type.NOT_PREFERRED, NAME, explain);
}

Expand All @@ -108,7 +120,6 @@ public Decision canAllocate(ShardRouting shardRouting, RoutingNode node, Routing
node.nodeId(),
newWriteThreadPoolUtilization
);
logger.trace(explanation);
return allocation.decision(Decision.YES, NAME, explanation);
}

Expand Down Expand Up @@ -146,7 +157,9 @@ public Decision canRemain(IndexMetadata indexMetadata, ShardRouting shardRouting
nodeWriteThreadPoolQueueLatencyThreshold.toHumanReadableString(2),
nodeWriteThreadPoolStats.averageThreadPoolUtilization()
);
logger.debug(explain);
if (logger.isDebugEnabled()) {
logInterventionMessage.maybeExecute(() -> logger.debug(explain));
}
return Decision.single(Decision.Type.NOT_PREFERRED, NAME, explain);
}

Expand All @@ -156,7 +169,6 @@ public Decision canRemain(IndexMetadata indexMetadata, ShardRouting shardRouting
nodeWriteThreadPoolStats.maxThreadPoolQueueLatencyMillis(),
nodeWriteThreadPoolQueueLatencyThreshold.toHumanReadableString(2)
);
logger.trace(explanation);
return allocation.decision(Decision.YES, NAME, explanation);
}

Expand Down
3 changes: 2 additions & 1 deletion server/src/main/java/org/elasticsearch/common/settings/ClusterSettings.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -650,6 +650,7 @@ public void apply(Settings value, Settings current, Settings previous) {
WriteLoadConstraintSettings.WRITE_LOAD_DECIDER_HIGH_UTILIZATION_THRESHOLD_SETTING,
WriteLoadConstraintSettings.WRITE_LOAD_DECIDER_HIGH_UTILIZATION_DURATION_SETTING,
WriteLoadConstraintSettings.WRITE_LOAD_DECIDER_QUEUE_LATENCY_THRESHOLD_SETTING,
WriteLoadConstraintSettings.WRITE_LOAD_DECIDER_REROUTE_INTERVAL_SETTING
WriteLoadConstraintSettings.WRITE_LOAD_DECIDER_REROUTE_INTERVAL_SETTING,
WriteLoadConstraintSettings.WRITE_LOAD_DECIDER_MINIMUM_LOGGING_INTERVAL
);
}