-
Notifications
You must be signed in to change notification settings - Fork 197
Add native memory circuit breaker. #689
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,15 +3,19 @@ | |
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| package org.opensearch.ml.common.breaker; | ||
|
|
||
| import lombok.extern.log4j.Log4j2; | ||
| import org.opensearch.monitor.jvm.JvmService; | ||
| package org.opensearch.ml.breaker; | ||
|
|
||
| import java.nio.file.Path; | ||
| import java.util.concurrent.ConcurrentHashMap; | ||
| import java.util.concurrent.ConcurrentMap; | ||
|
|
||
| import lombok.extern.log4j.Log4j2; | ||
|
|
||
| import org.opensearch.cluster.service.ClusterService; | ||
| import org.opensearch.common.settings.Settings; | ||
| import org.opensearch.monitor.jvm.JvmService; | ||
| import org.opensearch.monitor.os.OsService; | ||
|
|
||
| /** | ||
| * This service registers internal system breakers and provide API for users to register their own breakers. | ||
| */ | ||
|
|
@@ -20,14 +24,23 @@ public class MLCircuitBreakerService { | |
|
|
||
| private final ConcurrentMap<BreakerName, CircuitBreaker> breakers = new ConcurrentHashMap<>(); | ||
| private final JvmService jvmService; | ||
| private final OsService osService; | ||
| private final Settings settings; | ||
| private final ClusterService clusterService; | ||
|
|
||
| /** | ||
| * Constructor. | ||
| * | ||
| * @param jvmService jvm info | ||
| * @param osService os info | ||
| * @param settings settings | ||
| * @param clusterService clusterService | ||
| */ | ||
| public MLCircuitBreakerService(JvmService jvmService) { | ||
| public MLCircuitBreakerService(JvmService jvmService, OsService osService, Settings settings, ClusterService clusterService) { | ||
| this.jvmService = jvmService; | ||
| this.osService = osService; | ||
| this.settings = settings; | ||
| this.clusterService = clusterService; | ||
| } | ||
|
|
||
| public void registerBreaker(BreakerName name, CircuitBreaker breaker) { | ||
|
|
@@ -65,18 +78,21 @@ public MLCircuitBreakerService init(Path path) { | |
| log.info("Registered ML memory breaker."); | ||
| registerBreaker(BreakerName.DISK, new DiskCircuitBreaker(path.toString())); | ||
| log.info("Registered ML disk breaker."); | ||
| // Register native memory circuit breaker | ||
| registerBreaker(BreakerName.NATIVE_MEMORY, new NativeMemoryCircuitBreaker(this.osService, this.settings, this.clusterService)); | ||
| log.info("Registered ML native memory breaker."); | ||
|
|
||
| return this; | ||
| } | ||
|
|
||
| /** | ||
| * | ||
| * @return the name of any open circuit breaker; otherwise return null | ||
| * @return any open circuit breaker; otherwise return null | ||
| */ | ||
| public String checkOpenCB() { | ||
| public ThresholdCircuitBreaker checkOpenCB() { | ||
| for (CircuitBreaker breaker : breakers.values()) { | ||
| if (breaker.isOpen()) { | ||
| return breaker.getName(); | ||
| return (ThresholdCircuitBreaker) breaker; | ||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Why need to change the return type from
Collaborator
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Want to get more information like the current threshold when breaker is triggered to help debug.
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we can add a warning log to help |
||
| } | ||
| } | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| /* | ||
| * Copyright OpenSearch Contributors | ||
| * SPDX-License-Identifier: Apache-2.0 | ||
| */ | ||
|
|
||
| package org.opensearch.ml.breaker; | ||
|
|
||
| import static org.opensearch.ml.settings.MLCommonsSettings.ML_COMMONS_NATIVE_MEM_THRESHOLD; | ||
|
|
||
| import org.opensearch.cluster.service.ClusterService; | ||
| import org.opensearch.common.settings.Settings; | ||
| import org.opensearch.monitor.os.OsService; | ||
|
|
||
| /** | ||
| * A circuit breaker for native memory usage. | ||
| */ | ||
| public class NativeMemoryCircuitBreaker extends ThresholdCircuitBreaker<Short> { | ||
| private static final String ML_MEMORY_CB = "Native Memory Circuit Breaker"; | ||
| public static final short DEFAULT_NATIVE_MEM_USAGE_THRESHOLD = 90; | ||
| private final OsService osService; | ||
| private volatile Integer nativeMemThreshold = 90; | ||
|
|
||
| public NativeMemoryCircuitBreaker(OsService osService, Settings settings, ClusterService clusterService) { | ||
| super(DEFAULT_NATIVE_MEM_USAGE_THRESHOLD); | ||
| this.osService = osService; | ||
| this.nativeMemThreshold = ML_COMMONS_NATIVE_MEM_THRESHOLD.get(settings); | ||
| clusterService.getClusterSettings().addSettingsUpdateConsumer(ML_COMMONS_NATIVE_MEM_THRESHOLD, it -> nativeMemThreshold = it); | ||
| } | ||
|
|
||
| public NativeMemoryCircuitBreaker(Integer threshold, OsService osService) { | ||
| super(threshold.shortValue()); | ||
| this.nativeMemThreshold = threshold; | ||
| this.osService = osService; | ||
| } | ||
|
|
||
| @Override | ||
| public String getName() { | ||
| return ML_MEMORY_CB; | ||
| } | ||
|
|
||
| @Override | ||
| public Short getThreshold() { | ||
| return this.nativeMemThreshold.shortValue(); | ||
| } | ||
|
|
||
| @Override | ||
| public boolean isOpen() { | ||
| return osService.stats().getMem().getUsedPercent() > this.nativeMemThreshold.shortValue(); | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we just set threshold and compare directly without dividing
GBfor every request? Just like what you did for native memory breaker?