SegmentLoader refactoring#11466
Merged
abhishekagarwal87 merged 4 commits intoapache:masterfrom Jul 20, 2021
Merged
Conversation
abhishekagarwal87
force-pushed
the
segment_loader_refactoring
branch
from
a7c019d to
215c0f0
Compare
jihoonson
reviewed
Jul 20, 2021
Contributor
jihoonson
left a comment
jihoonson
left a comment
There was a problem hiding this comment.
The refactoring LGTM overall, but the CI failure seems legit. Please fix it.
[ERROR] testDefaultViewManagerBind(org.apache.druid.sql.guice.SqlModuleTest) Time elapsed: 0.017 s <<< ERROR!
com.google.inject.CreationException:
Unable to create injector, see the following errors:
- No implementation for org.apache.druid.segment.loading.SegmentLoader was bound.
while locating org.apache.druid.segment.loading.SegmentLoader
for the 1st parameter of org.apache.druid.server.SegmentManager.(SegmentManager.java:120)
while locating org.apache.druid.server.SegmentManager
for the 3rd parameter of org.apache.druid.sql.calcite.schema.DruidSchema.(DruidSchema.java:153)
while locating org.apache.druid.sql.calcite.schema.DruidSchema
for the 1st parameter of org.apache.druid.sql.calcite.schema.SystemSchema.(SystemSchema.java:216)
at org.apache.druid.sql.calcite.schema.DruidCalciteSchemaModule.configure(DruidCalciteSchemaModule.java:55) (via modules: org.apache.druid.sql.guice.SqlModule -> org.apache.druid.sql.calcite.schema.DruidCalciteSchemaModule)- No implementation for org.apache.druid.segment.loading.SegmentLoader was bound.
while locating org.apache.druid.segment.loading.SegmentLoader
for the 1st parameter of org.apache.druid.server.SegmentManager.(SegmentManager.java:120)
while locating org.apache.druid.server.SegmentManager
for the 3rd parameter of org.apache.druid.sql.calcite.schema.DruidSchema.(DruidSchema.java:153)
while locating org.apache.druid.sql.calcite.schema.DruidSchema
for the 1st parameter of org.apache.druid.sql.calcite.schema.NamedDruidSchema.(NamedDruidSchema.java:35)
at org.apache.druid.sql.guice.SqlBindings.addSchema(SqlBindings.java:58) (via modules: org.apache.druid.sql.guice.SqlModule -> org.apache.druid.sql.calcite.schema.DruidCalciteSchemaModule)
| this.indexIO = Preconditions.checkNotNull(indexIO, "null IndexIO"); | ||
| this.coordinatorClient = Preconditions.checkNotNull(coordinatorClient, "null CoordinatorClient"); | ||
| this.segmentLoaderFactory = Preconditions.checkNotNull(segmentLoaderFactory, "null SegmentLoaderFactory"); | ||
| this.segmentCacheManagerFactory = Preconditions.checkNotNull(segmentCacheManagerFactory, "null SegmentLoaderFactory"); |
Contributor
There was a problem hiding this comment.
Suggested change
| this.segmentCacheManagerFactory = Preconditions.checkNotNull(segmentCacheManagerFactory, "null SegmentLoaderFactory"); | |
| this.segmentCacheManagerFactory = Preconditions.checkNotNull(segmentCacheManagerFactory, "null segmentCacheManagerFactory"); |
| this.indexIO = Preconditions.checkNotNull(indexIO, "null IndexIO"); | ||
| this.coordinatorClient = Preconditions.checkNotNull(coordinatorClient, "null CoordinatorClient"); | ||
| this.segmentLoaderFactory = Preconditions.checkNotNull(segmentLoaderFactory, "null SegmentLoaderFactory"); | ||
| this.segmentCacheManagerFactory = Preconditions.checkNotNull(segmentCacheManagerFactory, "null SegmentLoaderFactory"); |
Contributor
There was a problem hiding this comment.
Suggested change
| this.segmentCacheManagerFactory = Preconditions.checkNotNull(segmentCacheManagerFactory, "null SegmentLoaderFactory"); | |
| this.segmentCacheManagerFactory = Preconditions.checkNotNull(segmentCacheManagerFactory, "null segmentCacheManagerFactory"); |
| public interface SegmentCacheManager | ||
| { | ||
| boolean isSegmentCached(DataSegment segment); | ||
| File getSegmentFiles(DataSegment segment) throws SegmentLoadingException; |
Contributor
There was a problem hiding this comment.
Perhaps nice to add a javadoc that says, this method can fetch the segment file unless it's already loaded.
Contributor
Author
|
Thanks @jihoonson for the review. I have addressed the comments and fixed the tests. |
jihoonson
reviewed
Jul 20, 2021
Contributor
jihoonson
left a comment
jihoonson
left a comment
There was a problem hiding this comment.
LGTM. Splitting interface makes sense to me.
jon-wei
approved these changes
Jul 20, 2021
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
4 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.
This PR splits current
SegmentLoaderintoSegmentLoaderandSegmentCacheManager.SegmentLoader- this class is responsible for building the segment object but does not expose any methods for downloading, cache space management, etc. Default implementation delegates the download operations toSegmentCacheManagerand only contains the logic for building segments once downloaded. . This class will be used inSegmentManagerto constructSegmentobjects.SegmentCacheManager- this class manages the segment cache on the local disk. It fetches the segment files to the local disk, can clean up the cache, and in the future, supportreserveandreleaseon cache space. [See https://github.com/Make SegmentLoader extensible and customizable #11398]. This class will be used in ingestion tasks such as compaction, re-indexing where segment files need to be downloaded locally.Key changed/added classes in this PR
SegmentLoaderSegmentCacheManagerSegmentLoadDropHandlerSegmentLocalCacheManagerSegmentLocalCacheLoaderSegmentLoaderFactory>SegmentCacheManagerFactoryThis PR has: