[BEAM-961] Add starting number to CountingInput #1505
Conversation
|
Refer to this link for build results (access rights to CI server needed): |
|
R: @jbonofre |
| * <p>To produce a bounded {@code PCollection<Long>} starting from {@code startOffset}, | ||
| * use {@link CountingInput#forSubrange(long, long)} : | ||
| * | ||
| * <pre>{@code |
There was a problem hiding this comment.
This code example is probably unnecessary - can just say at line 56, "use {@link ...} instead".
| * Creates a {@link BoundedCountingInput} that will produce the specified number of elements, | ||
| * starting from {@code startOffset} to (excluding) {@code endOffset}. | ||
| */ | ||
| public static BoundedCountingInput forSubrange(long startOffset, long endOffset) { |
There was a problem hiding this comment.
I think "index" might be a better name than "offset" here - offsets are usually measured relative to something important (e.g. the beginning of a file).
| @@ -76,6 +86,16 @@ public static BoundedCountingInput upTo(long numElements) { | |||
| } | |||
|
|
|||
| /** | |||
| * Creates a {@link BoundedCountingInput} that will produce the specified number of elements, | |||
There was a problem hiding this comment.
This overload does not let you specify the number of elements.
| * | ||
| * @deprecated use {@link CountingInput#forSubrange(long, long)} instead | ||
| */ | ||
| @Deprecated |
There was a problem hiding this comment.
Why add a method and immediately deprecate it? If you want users to not use it, package-local visibility is sufficient for that.
| @@ -83,6 +83,20 @@ | |||
| } | |||
|
|
|||
| /** | |||
| * Creates a {@link BoundedSource} that will produce the specified number of elements, | |||
There was a problem hiding this comment.
Same nitpicks about this comment.
| */ | ||
| @Deprecated | ||
| static BoundedSource<Long> createSourceForSubrange(long startOffset, long endOffset) { | ||
| checkArgument(endOffset > startOffset, "numElements (%s) must be greater than 0", |
There was a problem hiding this comment.
It's better to mention in an error message the things that the user directly specifies. Referring to numElements implies that the user specified a parameter called numElements, which is not the case. Rephrase as "endOffset must be greater than startOffset"?
| @@ -79,6 +79,26 @@ public static void addCountingAsserts(PCollection<Long> input, long numElements) | |||
| .isEqualTo(numElements - 1); | |||
| } | |||
|
|
|||
| public static void addCountingAsserts(PCollection<Long> input, long start, long end) { | |||
There was a problem hiding this comment.
Why this duplication of tests: what's the difference between CountingSourceTest and CountingInputTest?
There was a problem hiding this comment.
You're right, it's the same. I also refactored&removed some duplicate code from CountingInputTest
|
Refer to this link for build results (access rights to CI server needed): |
|
Looks good as far as I'm concerned; I'll let JB do the rest of the review, thanks! |
|
Minor drive-by comments:
|
Be sure to do all of the following to help us incorporate your contribution
quickly and easily:
[BEAM-<Jira issue #>] Description of pull requestmvn clean verify. (Even better, enableTravis-CI on your fork and ensure the whole test matrix passes).
<Jira issue #>in the title with the actual Jira issuenumber, if there is one.
Individual Contributor License Agreement.