Introduce RemoteIndexClient #2548
Conversation
owenhalpert
force-pushed
the
owen-remote-vector-dev
branch
from
57290c1 to
23faf72
Compare
| public static final long BASE_DELAY_MS = 100; | ||
| public static final long INITIAL_DELAY_MS = 10; |
There was a problem hiding this comment.
These are currently set low for testing, though we haven't thought on strong defaults for these anyway.
owenhalpert
force-pushed
the
owen-remote-vector-dev
branch
from
23faf72 to
9bb2d92
Compare
| long pollInterval = ((TimeValue) (KNNSettings.state().getSettingValue(KNNSettings.KNN_REMOTE_BUILD_SERVICE_POLL_INTERVAL))) | ||
| .getMillis(); | ||
|
|
||
| Thread.sleep(INITIAL_DELAY_MS); |
There was a problem hiding this comment.
I need to look into this some more, but I'm not sure this is the best implementation. Off the top of my head I am thinking of some sort of job scheduler semantic with a latch and we can do latch.await() here or something like that. Also it seems a little dangerous to do this in a Singleton class.
There was a problem hiding this comment.
Also, shouldn't INITIAL_DELAY_MS just be pollInterval? or maybe some function of it.
There was a problem hiding this comment.
I don't see the two as related. I was thinking of the INITIAL_DELAY_MS as a buffer to prevent the client from sending any status requests when we know for sure that the build is still in progress. For example finding a lower bound t on the GPU build or calculating this based on workload size, then waiting 0.5t so to not send any unnecessary status checks.
There was a problem hiding this comment.
Thinking on it some more I think Thread.sleep is fine, but let's make sure we have tests for it across concurrent jobs.
For the other point, the polling interval itself should also be a function of t else it's also going to send unnecessary status checks.
| * @param jobId to check | ||
| * @return HttpExecuteResponse for the status request | ||
| */ | ||
| public String getBuildStatus(String jobId) throws IOException, URISyntaxException { |
There was a problem hiding this comment.
I think a lot of these methods can be private or protected
owenhalpert
force-pushed
the
owen-remote-vector-dev
branch
from
9bb2d92 to
907c6be
Compare
There was a problem hiding this comment.
Thanks @owenhalpert!
At a high level I think we should separate out the client changes into the following individual PRs:
- Client skeleton + build request, including polling logic + tests, failure handling
- Polling + failure handling
- Metrics
I think there is a lot more testing that needs to be written for all of these and I think it will be much easier to focus on each component individually.
src/main/java/org/opensearch/knn/index/codec/nativeindex/remote/RemoteIndexBuildStrategy.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/knn/index/remote/RemoteIndexClient.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/knn/index/remote/RemoteIndexClient.java
Outdated
Show resolved
Hide resolved
src/main/java/org/opensearch/knn/index/remote/RemoteIndexClient.java
Outdated
Show resolved
Hide resolved
| long pollInterval = ((TimeValue) (KNNSettings.state().getSettingValue(KNNSettings.KNN_REMOTE_BUILD_SERVICE_POLL_INTERVAL))) | ||
| .getMillis(); | ||
|
|
||
| Thread.sleep(INITIAL_DELAY_MS); |
There was a problem hiding this comment.
Thinking on it some more I think Thread.sleep is fine, but let's make sure we have tests for it across concurrent jobs.
For the other point, the polling interval itself should also be a function of t else it's also going to send unnecessary status checks.
owenhalpert
force-pushed
the
owen-remote-vector-dev
branch
2 times, most recently
from
5017998 to
d5eeec9
Compare
Add RemoteIndexClient initial implementation, its accompanying dependencies, and Build Request, Retry Strategy, and test files Signed-off-by: owenhalpert <[email protected]>
owenhalpert
force-pushed
the
owen-remote-vector-dev
branch
from
d5eeec9 to
c50fdbf
Compare
Description
First PR for #2518
This PR introduces the initial implementation of the Remote Index client, which is awaiting implementation on the following:
Related Issues
#2391 Meta Issue
#2393 HLD by @jed326
#2518 My LLD
Check List
- [ ] New functionality has been documented.- [ ] API changes companion pull request created.--signoff.- [ ] Public documentation issue/PR created.By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.