Refactor IO thread pool usage #3007
Merged
Conversation
This file contains 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
pomadchin
force-pushed
the
feature/thread-pool
branch
from
3eb37d0 to
691de3a
Compare
pomadchin
force-pushed
the
feature/thread-pool
branch
2 times, most recently
from
115e010 to
109a225
Compare
pomadchin
force-pushed
the
feature/thread-pool
branch
3 times, most recently
from
0914285 to
d81ecd0
Compare
echeipesh
reviewed
Jun 27, 2019
| abstract class COGCollectionLayerReader[ID] { self => | ||
| implicit val ec: ExecutionContext |
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 .
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.
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)
s3/src/main/scala/geotrellis/store/s3/cog/S3COGCollectionLayerReader.scala
Outdated
Show resolved
Hide resolved
spark/src/main/scala/geotrellis/spark/store/file/FileRDDReader.scala
Outdated
Show resolved
Hide resolved
spark/src/test/scala/geotrellis/spark/store/hadoop/util/HdfsRangeReaderProviderSpec.scala
Show resolved
Hide resolved
store/src/main/scala/geotrellis/store/file/FileCollectionLayerProvider.scala
Show resolved
Hide resolved
pomadchin
force-pushed
the
feature/thread-pool
branch
2 times, most recently
from
430c683 to
f4e9b05
Compare
pomadchin
force-pushed
the
feature/thread-pool
branch
from
f4e9b05 to
675235c
Compare
pomadchin
force-pushed
the
feature/thread-pool
branch
from
675235c to
661530d
Compare
echeipesh
reviewed
Jul 3, 2019
echeipesh
reviewed
Jul 3, 2019
store/src/main/scala/geotrellis/store/file/FileCollectionLayerReader.scala
Outdated
Show resolved
Hide resolved
echeipesh
reviewed
Jul 3, 2019
store/src/main/scala/geotrellis/store/conf/BlockingThreadPoolConfig.scala
Outdated
Show resolved
Hide resolved
echeipesh
reviewed
Jul 3, 2019
echeipesh
approved these changes
Jul 8, 2019
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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