Repository navigation
Refactor IO thread pool usage - #3007
Merged
Merged
Conversation
pomadchin
force-pushed
the
feature/thread-pool
branch
from
June 25, 2019 21:18
3eb37d0 to
691de3a
Compare
pomadchin
force-pushed
the
feature/thread-pool
branch
2 times, most recently
from
June 25, 2019 23:48
115e010 to
109a225
Compare
pomadchin
force-pushed
the
feature/thread-pool
branch
3 times, most recently
from
June 27, 2019 13:04
0914285 to
d81ecd0
Compare
echeipesh
reviewed
Jun 27, 2019
|
|
||
|
|
||
| abstract class COGCollectionLayerReader[ID] { self => | ||
| implicit val ec: ExecutionContext |
Contributor
There was a problem hiding this comment.
This is weird for two reasons.
COGCollectionLayerReaderis already abstract class, why not just make it a constructor parameter, that is way more explicit- Is it very painful to have this be non
implicit? Its a little difficult to see where it is used .
Member
Author
There was a problem hiding this comment.
For the COGCollectionLayerReader.read I think it makes not a lot of sense to pass execution context explicitly in the code that can work only 'locally' and is internal API in fact.
Member
Author
There was a problem hiding this comment.
Also do I understand correct that your 1. comment can be applied to all our abstract classes? (CollectionLayerReader for instance)
pomadchin
force-pushed
the
feature/thread-pool
branch
2 times, most recently
from
July 1, 2019 23:13
430c683 to
f4e9b05
Compare
pomadchin
force-pushed
the
feature/thread-pool
branch
from
July 1, 2019 23:25
f4e9b05 to
675235c
Compare
pomadchin
force-pushed
the
feature/thread-pool
branch
from
July 2, 2019 12:33
675235c to
661530d
Compare
echeipesh
reviewed
Jul 3, 2019
echeipesh
reviewed
Jul 3, 2019
echeipesh
reviewed
Jul 3, 2019
echeipesh
reviewed
Jul 3, 2019
echeipesh
approved these changes
Jul 8, 2019
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Overview
This PR refactors GeoTrellis blocking thread pool usages. Instead of having multiple thread pools for each particular backend / reader / writer there would be a single and configurable thread pool. By default it is a
FixedThreadPoolwith ageotrellis-default-io-%dname. In addition to that, it was decided to make it configurable by user. This PR allows users to use their own thread pools for all kind of readers / writers.In this PR we also introduced a new way to pass objects that should not be serialized (
ExecutionContextandS3Client) into objects constructors. Instead of passing a function to create an instance of a thing, it was decided to pass these objects asby-nameparameters. Such approach allows to simplify the API and makes the API more transparent.This PR touches all the project configuration files, and with this PR we introduce
hyphen-separatedcase usage by default instead of acamel-case.docs/CHANGELOG.rstupdated, if necessaryCloses #2945