Move/rename some stray Spark datasource classes, and delete BaseRecalibratorSparkSharded #5192
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
…ibratorSparkSharded -Move the Spark reference datasource classes from the engine.datasources package into the engine.spark.datasources package, and rename them to make it clear that they are for use on Spark. This fixes a longstanding problem where they were getting confused with the walker ReferenceDataSource class. -Delete the unused/unmaintained experimental tool BaseRecalibratorSparkSharded, which has fallen out-of-date relative to BaseRecalibratorSpark. -Delete an extra "VariantSource" class that is now unused (note: this is not the same as VariantSparkSource, which is used extensively and retained here)
|
@jamesemery please review |
jamesemery
reviewed
Sep 14, 2018
| @@ -21,33 +20,33 @@ | |||
| * | |||
| * This class needs to subclassed by test code, so it cannot be declared final. | |||
| */ | |||
| public class ReferenceMultiSource implements ReferenceSource, Serializable { | |||
| public class ReferenceMultiSparkSource implements ReferenceSparkSource, Serializable { | |||
There was a problem hiding this comment.
I don't like this name, this should really be ReferenceSparkSource as the multi is confusing (are there multiple sparks?"
There was a problem hiding this comment.
I don't like it either, but ReferenceSparkSource is taken already as the interface name. I think it's better to have a consistent naming convention across these classes for now. We can improve the names if we ever merge the walker and Spark datasource classes.
|
|
||
| Assert.assertTrue(CollectionUtils.isEqualCollection(rddParallelVariants.collect(), variantsList)); | ||
| } | ||
|
|
||
| /** | ||
| * getVariantsListAs grabs the variants from local files (or perhaps eventually buckets), applies |
There was a problem hiding this comment.
Do we really need javadocs on a private method used by a handful of tests in this class?
There was a problem hiding this comment.
Not really, but it doesn't hurt.
| @@ -10,15 +9,15 @@ | |||
| /** | |||
| * Internal interface to load a reference sequence. | |||
| */ | |||
| public interface ReferenceSource { | |||
| public interface ReferenceSparkSource { | |||
There was a problem hiding this comment.
SparkReferenceSource or ReferenceDataSparkSource
There was a problem hiding this comment.
These seem inconsistent with the convention already in use across the engine.spark.datasources package. Again, I think it's better to stick to a consistent naming convention for now, since the goal of this PR is to disambiguate these classes vs. their walker counterparts.
* Move/rename ReferenceMultiSparkSourceUnitTest * Delete unused AddContextDataToReadSparkOptimized
|
Added a second commit to delete the now-unused |
tomwhite
added a commit
that referenced
this pull request
Oct 8, 2018
* AddContextDataToReadSpark (and AddContextDataToReadSparkUnitTest) implemented the different JoinStrategy options for BQSR; has been replaced with the Spark Files mecahnism (see #5127) * BroadcastJoinReadsWithRefBases and JoinReadsWithRefBasesSparkUnitTest were only used by AddContextDataToReadSpark * BroadcastJoinReadsWithVariants and JoinReadsWithVariantsSparkUnitTest were only used by AddContextDataToReadSpark * ShuffleJoinReadsWithRefBases and ShuffleJoinReadsWithVariants were only used by AddContextDataToReadSpark * JoinStrategy was only used for BQSR (HC always uses overlaps partitioner), but is no longer used since #5127 * KnownSitesCache was replaced with Spark Files * ReferenceMultiSourceAdapter in HaplotypeCallerSpark was replaced with the regular ReferenceDataSource * BaseRecalibratorEngineSparkWrapper was only used by BaseRecalibratorSparkSharded, which was removed in #5192
jamesemery
pushed a commit
that referenced
this pull request
Jan 11, 2019
* AddContextDataToReadSpark (and AddContextDataToReadSparkUnitTest) implemented the different JoinStrategy options for BQSR; has been replaced with the Spark Files mecahnism (see #5127) * BroadcastJoinReadsWithRefBases and JoinReadsWithRefBasesSparkUnitTest were only used by AddContextDataToReadSpark * BroadcastJoinReadsWithVariants and JoinReadsWithVariantsSparkUnitTest were only used by AddContextDataToReadSpark * ShuffleJoinReadsWithRefBases and ShuffleJoinReadsWithVariants were only used by AddContextDataToReadSpark * JoinStrategy was only used for BQSR (HC always uses overlaps partitioner), but is no longer used since #5127 * KnownSitesCache was replaced with Spark Files * ReferenceMultiSourceAdapter in HaplotypeCallerSpark was replaced with the regular ReferenceDataSource * BaseRecalibratorEngineSparkWrapper was only used by BaseRecalibratorSparkSharded, which was removed in #5192
jamesemery
pushed a commit
that referenced
this pull request
Jan 11, 2019
* AddContextDataToReadSpark (and AddContextDataToReadSparkUnitTest) implemented the different JoinStrategy options for BQSR; has been replaced with the Spark Files mecahnism (see #5127) * BroadcastJoinReadsWithRefBases and JoinReadsWithRefBasesSparkUnitTest were only used by AddContextDataToReadSpark * BroadcastJoinReadsWithVariants and JoinReadsWithVariantsSparkUnitTest were only used by AddContextDataToReadSpark * ShuffleJoinReadsWithRefBases and ShuffleJoinReadsWithVariants were only used by AddContextDataToReadSpark * JoinStrategy was only used for BQSR (HC always uses overlaps partitioner), but is no longer used since #5127 * KnownSitesCache was replaced with Spark Files * ReferenceMultiSourceAdapter in HaplotypeCallerSpark was replaced with the regular ReferenceDataSource * BaseRecalibratorEngineSparkWrapper was only used by BaseRecalibratorSparkSharded, which was removed in #5192
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.
-Move the Spark reference datasource classes from the engine.datasources package into the
engine.spark.datasources package, and rename them to make it clear that they are for use
on Spark. This fixes a longstanding problem where they were getting confused with the
walker ReferenceDataSource/ReferenceFileSource classes.
-Delete the unused/unmaintained experimental tool BaseRecalibratorSparkSharded, which has
fallen out-of-date relative to BaseRecalibratorSpark, as well as its unused companion
AddContextDataToReadsSparkOptimized.-Delete an extra "VariantSource" class that is now unused (note: this is not the same as
VariantSparkSource, which is used extensively and retained here)