Add option for downloader to listen on unix socket #2102
Conversation
|
|
||
| if (s3Configuration.getLocalDownloadSocket().isPresent()) { | ||
| UnixSocketConnector unix = new UnixSocketConnector(server); | ||
| unix.setAcceptQueueSize(128); |
There was a problem hiding this comment.
Not sure if you want to read this from /proc/sys/net/core/somaxconn or otherwise provide a way to override this.
There was a problem hiding this comment.
Good idea, will make it a configurable value for the moment
| server.addConnector(http); | ||
| } | ||
|
|
||
| if (s3Configuration.getLocalDownloadSocket().isPresent()) { |
There was a problem hiding this comment.
is this is an if+if instead of an if+else so that http can be used as a fallback for unix sockets?
There was a problem hiding this comment.
if + if, both can be enabled at once (for ease of migration over to the new one)
| this.s3Artifact = s3Artifact; | ||
| } | ||
|
|
||
| public ContentResponse getReponse() throws InterruptedException { |
| SingularityExecutorTask task | ||
| ) | ||
| throws InterruptedException { | ||
| final List<CompletableFutureHolder> futures = Lists.newArrayListWithCapacity( |
There was a problem hiding this comment.
Could possibly refactor the two local download fetchers into a single implementation that takes in an S3Artifact -> CompletableFuture function, if you think consolidating the implementations is worth it.
There was a problem hiding this comment.
The ning async client works with the other futures by default. Didn't want to mess with the existing/working code as much as possible for the moment. Agree CompletableFuture is much nicer though
Gives the ability to better lock down the s3 downloader. If a unix socket is set in the base configuration, the downloader will listen on the given socket and the executor will use that to trigger downloads.
Still need to test