-
Notifications
You must be signed in to change notification settings - Fork 33
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
Add rate limiter for bulk request #567
Changes from 3 commits
0a8e02d
7ead4a2
1d493b2
25d3f97
30bc945
0a71be4
8576224
54d8cf1
406f57e
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 |
---|---|---|
|
@@ -37,6 +37,7 @@ | |
import org.opensearch.client.transport.rest_client.RestClientTransport; | ||
|
||
import java.io.IOException; | ||
import org.opensearch.flint.core.storage.BulkRequestRateLimiter; | ||
|
||
import static org.opensearch.flint.core.metrics.MetricConstants.OS_READ_OP_METRIC_PREFIX; | ||
import static org.opensearch.flint.core.metrics.MetricConstants.OS_WRITE_OP_METRIC_PREFIX; | ||
|
@@ -47,6 +48,7 @@ | |
*/ | ||
public class RestHighLevelClientWrapper implements IRestHighLevelClient { | ||
private final RestHighLevelClient client; | ||
private final BulkRequestRateLimiter rateLimiter; | ||
|
||
private final static JacksonJsonpMapper JACKSON_MAPPER = new JacksonJsonpMapper(); | ||
|
||
|
@@ -55,13 +57,21 @@ public class RestHighLevelClientWrapper implements IRestHighLevelClient { | |
* | ||
* @param client the RestHighLevelClient instance to wrap | ||
*/ | ||
public RestHighLevelClientWrapper(RestHighLevelClient client) { | ||
public RestHighLevelClientWrapper(RestHighLevelClient client, BulkRequestRateLimiter rateLimiter) { | ||
this.client = client; | ||
this.rateLimiter = rateLimiter; | ||
} | ||
|
||
@Override | ||
public BulkResponse bulk(BulkRequest bulkRequest, RequestOptions options) throws IOException { | ||
return execute(OS_WRITE_OP_METRIC_PREFIX, () -> client.bulk(bulkRequest, options)); | ||
return execute(OS_WRITE_OP_METRIC_PREFIX, () -> { | ||
try { | ||
rateLimiter.acquirePermit(); | ||
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.
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.
|
||
return client.bulk(bulkRequest, options); | ||
} catch (InterruptedException e) { | ||
throw new RuntimeException("rateLimiter.acquirePermit was interrupted.", e); | ||
} | ||
}); | ||
} | ||
|
||
@Override | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,30 @@ | ||
package org.opensearch.flint.core.storage; | ||
|
||
import dev.failsafe.RateLimiter; | ||
import java.time.Duration; | ||
import java.util.logging.Logger; | ||
import org.opensearch.flint.core.FlintOptions; | ||
|
||
public class BulkRequestRateLimiter { | ||
private static final Logger LOG = Logger.getLogger(BulkRequestRateLimiter.class.getName()); | ||
private RateLimiter<Void> rateLimiter; | ||
|
||
public BulkRequestRateLimiter(FlintOptions flintOptions) { | ||
long bulkRequestRateLimitPerNode = flintOptions.getBulkRequestRateLimitPerNode(); | ||
if (bulkRequestRateLimitPerNode > 0) { | ||
LOG.info("Setting rate limit for bulk request to " + bulkRequestRateLimitPerNode + "/sec"); | ||
this.rateLimiter = RateLimiter.<Void>smoothBuilder( | ||
flintOptions.getBulkRequestRateLimitPerNode(), | ||
Duration.ofSeconds(1)).build(); | ||
} else { | ||
LOG.info("Rate limit for bulk request was not set."); | ||
} | ||
} | ||
|
||
// Wait so it won't exceed rate limit. Does nothing if rate limit is not set. | ||
public void acquirePermit() throws InterruptedException { | ||
if (rateLimiter != null) { | ||
this.rateLimiter.acquirePermit(); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,22 @@ | ||
package org.opensearch.flint.core.storage; | ||
|
||
import org.opensearch.flint.core.FlintOptions; | ||
|
||
/** | ||
* Hold shared instance of BulkRequestRateLimiter. This class is introduced to make | ||
* BulkRequestRateLimiter testable and share single instance. | ||
*/ | ||
public class BulkRequestRateLimiterHolder { | ||
|
||
private static BulkRequestRateLimiter instance; | ||
|
||
private BulkRequestRateLimiterHolder() {} | ||
|
||
public synchronized static BulkRequestRateLimiter getBulkRequestRateLimiter( | ||
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. does Holder necessary? move to BulkRequestRateLimiter? 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. As written in the comment, it is needed to use shared single instance, and make BulkRequestRateLimiter testable. If we directly make BulkRequestRateLimiter singleton, we cannot test it with different parameters. |
||
FlintOptions flintOptions) { | ||
if (instance == null) { | ||
instance = new BulkRequestRateLimiter(flintOptions); | ||
} | ||
return instance; | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
package org.opensearch.flint.core.storage; | ||
|
||
import static org.junit.jupiter.api.Assertions.assertEquals; | ||
import static org.junit.jupiter.api.Assertions.assertNotNull; | ||
|
||
import com.google.common.collect.ImmutableMap; | ||
import org.junit.jupiter.api.Test; | ||
import org.opensearch.flint.core.FlintOptions; | ||
|
||
class BulkRequestRateLimiterHolderTest { | ||
FlintOptions flintOptions = new FlintOptions(ImmutableMap.of()); | ||
@Test | ||
public void getBulkRequestRateLimiter() { | ||
BulkRequestRateLimiter instance0 = BulkRequestRateLimiterHolder.getBulkRequestRateLimiter(flintOptions); | ||
BulkRequestRateLimiter instance1 = BulkRequestRateLimiterHolder.getBulkRequestRateLimiter(flintOptions); | ||
|
||
assertNotNull(instance0); | ||
assertEquals(instance0, instance1); | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,46 @@ | ||
package org.opensearch.flint.core.storage; | ||
|
||
|
||
import static org.junit.jupiter.api.Assertions.assertEquals; | ||
import static org.junit.jupiter.api.Assertions.assertNotNull; | ||
import static org.junit.jupiter.api.Assertions.assertTrue; | ||
|
||
import com.google.common.collect.ImmutableMap; | ||
import org.junit.jupiter.api.Test; | ||
import org.opensearch.flint.core.FlintOptions; | ||
|
||
class BulkRequestRateLimiterTest { | ||
FlintOptions flintOptionsWithRateLimit = new FlintOptions(ImmutableMap.of(FlintOptions.BULK_REQUEST_RATE_LIMIT_PER_NODE, "1")); | ||
FlintOptions flintOptionsWithoutRateLimit = new FlintOptions(ImmutableMap.of(FlintOptions.BULK_REQUEST_RATE_LIMIT_PER_NODE, "0")); | ||
|
||
@Test | ||
void acquirePermitWithRateConfig() throws Exception { | ||
BulkRequestRateLimiter limiter = new BulkRequestRateLimiter(flintOptionsWithRateLimit); | ||
|
||
assertTrue(timer(() -> { | ||
limiter.acquirePermit(); | ||
limiter.acquirePermit(); | ||
}) >= 1000); | ||
} | ||
|
||
@Test | ||
void acquirePermitWithoutRateConfig() throws Exception { | ||
BulkRequestRateLimiter limiter = new BulkRequestRateLimiter(flintOptionsWithoutRateLimit); | ||
|
||
assertTrue(timer(() -> { | ||
limiter.acquirePermit(); | ||
limiter.acquirePermit(); | ||
}) < 100); | ||
} | ||
|
||
private interface Procedure { | ||
void run() throws Exception; | ||
} | ||
|
||
private long timer(Procedure procedure) throws Exception { | ||
long start = System.currentTimeMillis(); | ||
procedure.run(); | ||
long end = System.currentTimeMillis(); | ||
return end - start; | ||
} | ||
} |
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.
0.1 means 1 request per 10 seconds? could u add more in doc.
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.
It won't accept decimal value. We could support if we modify the rate limit period, but I think it would become complicated considering someone might specify a value like
1.23
. If we want to reduce the traffic less than 1 request/sec, we can reduce the size of batch instead. It would reduce the actual number of request.