-
Notifications
You must be signed in to change notification settings - Fork 10
Async search context and and state management framework #6
Changes from 5 commits
2b8bbfd
db91f3b
a5f9580
073d4a9
3c692c1
4e9663e
5fff307
8ab0c79
2faec54
1fc0a1e
7a6800d
b11868f
ac9674d
5863650
2212a49
e69d4a5
57010e4
c42a15b
0438193
d6358f2
1a16c0a
5a3204b
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 |
---|---|---|
@@ -0,0 +1,68 @@ | ||
/* | ||
* Copyright 2020 Amazon.com, Inc. or its affiliates. All Rights Reserved. | ||
* | ||
* Licensed under the Apache License, Version 2.0 (the "License"). | ||
* You may not use this file except in compliance with the License. | ||
* A copy of the License is located at | ||
* | ||
* http://www.apache.org/licenses/LICENSE-2.0 | ||
* | ||
* or in the "license" file accompanying this file. This file is distributed | ||
* on an "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either | ||
* express or implied. See the License for the specific language governing | ||
* permissions and limitations under the License. | ||
*/ | ||
|
||
package com.amazon.opendistroforelasticsearch.search.async; | ||
|
||
import com.amazon.opendistroforelasticsearch.search.async.context.AsyncSearchContextId; | ||
import org.apache.lucene.store.ByteArrayDataInput; | ||
import org.elasticsearch.common.bytes.BytesReference; | ||
import org.elasticsearch.common.io.stream.BytesStreamOutput; | ||
|
||
import java.io.IOException; | ||
import java.util.Base64; | ||
|
||
public class AsyncSearchIdConverter { | ||
/** | ||
* Encodes the {@linkplain AsyncSearchId} in base64 encoding and returns an identifier for the submitted async search | ||
* | ||
* @param asyncSearchId The object to be encoded | ||
* @return The id which is used to access the submitted async search | ||
*/ | ||
public static String buildAsyncId(AsyncSearchId asyncSearchId) { | ||
try (BytesStreamOutput out = new BytesStreamOutput()) { | ||
out.writeString(asyncSearchId.getNode()); | ||
out.writeLong(asyncSearchId.getTaskId()); | ||
out.writeString(asyncSearchId.getAsyncSearchContextId().getContextId()); | ||
out.writeLong(asyncSearchId.getAsyncSearchContextId().getId()); | ||
return Base64.getUrlEncoder().withoutPadding().encodeToString(BytesReference.toBytes(out.bytes())); | ||
} catch (IOException e) { | ||
throw new IllegalArgumentException("Cannot build async search id", e); | ||
} | ||
} | ||
|
||
/** | ||
* Attempts to decode a base64 encoded string into an {@linkplain AsyncSearchId} which contains the details pertaining to | ||
* an async search being accessed. | ||
* | ||
* @param asyncSearchId The string to be decoded | ||
* @return The parsed {@linkplain AsyncSearchId} | ||
*/ | ||
public static AsyncSearchId parseAsyncId(String asyncSearchId) { | ||
try { | ||
byte[] bytes = Base64.getUrlDecoder().decode(asyncSearchId); | ||
ByteArrayDataInput in = new ByteArrayDataInput(bytes); | ||
String node = in.readString(); | ||
long taskId = in.readLong(); | ||
String contextId = in.readString(); | ||
long id = in.readLong(); | ||
if (in.getPosition() != bytes.length) { | ||
throw new IllegalArgumentException("Not all bytes were read"); | ||
} | ||
return new AsyncSearchId(node, taskId, new AsyncSearchContextId(contextId, id)); | ||
} catch (Exception e) { | ||
throw new IllegalArgumentException("Cannot parse async search id", e); | ||
} | ||
} | ||
} |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,6 +28,7 @@ | |
import org.elasticsearch.common.util.concurrent.RunOnce; | ||
import org.elasticsearch.threadpool.ThreadPool; | ||
|
||
import java.io.Closeable; | ||
import java.util.concurrent.Semaphore; | ||
import java.util.concurrent.TimeUnit; | ||
import java.util.concurrent.TimeoutException; | ||
|
@@ -38,15 +39,16 @@ | |
* before it transitions context to the index. Provides fairness to consumers and throws {@linkplain TimeoutException} after | ||
* maximum time has elapsed waiting for the in-flight operations block. | ||
*/ | ||
public class AsyncSearchContextPermits { | ||
public class AsyncSearchContextPermits implements Closeable { | ||
|
||
private static final int TOTAL_PERMITS = Integer.MAX_VALUE; | ||
|
||
//visible for testing | ||
final Semaphore semaphore; | ||
private final AsyncSearchContextId asyncSearchContextId; | ||
private volatile String lockDetails; | ||
private final ThreadPool threadPool; | ||
private volatile boolean closed; | ||
|
||
private static final Logger logger = LogManager.getLogger(AsyncSearchContextPermits.class); | ||
|
||
public AsyncSearchContextPermits(AsyncSearchContextId asyncSearchContextId, ThreadPool threadPool) { | ||
|
@@ -56,24 +58,27 @@ public AsyncSearchContextPermits(AsyncSearchContextId asyncSearchContextId, Thre | |
} | ||
|
||
private Releasable acquirePermits(int permits, TimeValue timeout, final String details) throws TimeoutException { | ||
RunOnce release = new RunOnce(() -> {}); | ||
if (closed) { | ||
throw new IllegalStateException("trying to acquire permits on closed context ["+ asyncSearchContextId +"]"); | ||
} | ||
try { | ||
if (semaphore.tryAcquire(permits, timeout.getMillis(), TimeUnit.MILLISECONDS)) { | ||
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. What is the advantage of keeping a timeout here, considering the total permits is integer max. There are definite gains of not using timeout though. One use case I can think is for mutating operations contending for permit while all is acquired by post processor? 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. The timeout is because there is also an |
||
this.lockDetails = details; | ||
final RunOnce release = new RunOnce(() -> semaphore.release(permits)); | ||
release = new RunOnce(() -> semaphore.release(permits)); | ||
return release::run; | ||
} else { | ||
throw new TimeoutException( | ||
"obtaining context lock" + asyncSearchContextId + "timed out after " + timeout.getMillis() + "ms, " + | ||
"previous lock details: [" + lockDetails + "] trying to lock for [" + details + "]"); | ||
throw new TimeoutException("obtaining context lock" + asyncSearchContextId + "timed out after " + | ||
timeout.getMillis() + "ms, previous lock details: [" + lockDetails + "] trying to lock for [" + details + "]"); | ||
} | ||
} catch (InterruptedException e) { | ||
Thread.currentThread().interrupt(); | ||
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. What about permit release? Do you want to handle the interrupts upstream. That would ensure the release of the permit as well. 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. Possibly we don't think we need to worry about releasing permit here, as one is not acquired yet. 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. Added an explicit release on interrupts |
||
release.run(); | ||
throw new RuntimeException("thread interrupted while trying to obtain context lock", e); | ||
} | ||
} | ||
|
||
private void asyncAcquirePermit( | ||
int permits, final ActionListener<Releasable> onAcquired, final TimeValue timeout, String reason) { | ||
private void asyncAcquirePermit(int permits, final ActionListener<Releasable> onAcquired, final TimeValue timeout, String reason) { | ||
threadPool.executor(AsyncSearchPlugin.OPEN_DISTRO_ASYNC_SEARCH_GENERIC_THREAD_POOL_NAME).execute(new AbstractRunnable() { | ||
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. Do we plan to introduce a separate threadpool going forward? 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. Not sure which thread pool you mean, while we might add few based on our performance tests 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. Dedicated threadpool for different asyc search operations. Agree that can wait for more concrete results based on perf testing. |
||
@Override | ||
public void onFailure(final Exception e) { | ||
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. handlePermit release? 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. This method is invoked for all exception thrown by
|
||
|
@@ -118,4 +123,9 @@ public void asyncAcquirePermit(final ActionListener<Releasable> onAcquired, fina | |
public void asyncAcquireAllPermits(final ActionListener<Releasable> onAcquired, final TimeValue timeout, String reason) { | ||
asyncAcquirePermit(TOTAL_PERMITS, onAcquired, timeout, reason); | ||
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. [Question] I don't really get the logic of Integer.MAX_VALUE. 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. When we acquire a single permit out of Integer.MAX_VALUE number of available permits, we imply that its a non When we acquire all available permits, we imply that its a blocking operation.( |
||
} | ||
|
||
@Override | ||
public void close() { | ||
closed = true; | ||
} | ||
} |
This file was deleted.
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.
remove this?
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.
Will check on this. Will revisit with more tests