[BEAM-1792] Changing Spark to use MetricFiltering#2304
Conversation
pabloem
commented
Mar 23, 2017
pabloem
commented
- Make sure the PR title is formatted like: [BEAM-<Jira issue #>] Description of pull request
- Make sure tests pass via mvn clean verify. (Even better, enable Travis-CI on your fork and ensure the whole test matrix passes).
- Replace <Jira issue #> in the title with the actual Jira issue number, if there is one.
- If this contribution is large, please file an Apache Individual Contributor License Agreement.
|
Changes Unknown when pulling a1a5ddc on pabloem:fix-spark-filtering into ** on apache:master**. |
|
Refer to this link for build results (access rights to CI server needed): |
|
r: @aviemzur |
|
Run Spark RunnableOnService |
|
Refer to this link for build results (access rights to CI server needed): Failed Tests: 1beam_PostCommit_Java_RunnableOnService_Spark/org.apache.beam:beam-runners-spark: 1
--none-- |
aviemzur
left a comment
aviemzur
left a comment
There was a problem hiding this comment.
Looks great!
Only have one comment.
The test failure seems unrelated, we seem to have a broken build regardless, but let's hold on the merge until this is fixed since the test also uses metrics.
| checkNotNull(namespace, "Must specify a inNamespace"); | ||
| checkNotNull(name, "Must specify a name"); | ||
| return new AutoValue_MetricNameFilter(namespace.getSimpleName(), name); | ||
| return new AutoValue_MetricNameFilter(namespace.getName(), name); |
There was a problem hiding this comment.
This seems to be a positive change, but I'm not sure it is needed for this PR.
I'm assuming this change was done to match https://github.com/apache/beam/blob/master/sdks/java/core/src/main/java/org/apache/beam/sdk/metrics/MetricName.java#L45-L45
There seems to be no test coverage for this. I'd add tests as well (Although again, probably in a separate PR).
There was a problem hiding this comment.
That's fair. I've split this up into separate PRs.
pabloem
force-pushed
the
fix-spark-filtering
branch
from
a1a5ddc to
ca5aec9
Compare
|
Refer to this link for build results (access rights to CI server needed): |
|
@aviemzur the build is fine now, it seems. If it looks fine to you, could you merge it? (If not, I can ask Ben, just let me know if the changes look okay ;) |
|
Yeah, looks good. |
pabloem
force-pushed
the
fix-spark-filtering
branch
from
ca5aec9 to
fb0e99d
Compare
|
Refer to this link for build results (access rights to CI server needed): |
|
Run Spark RunnableOnService |
|
Refer to this link for build results (access rights to CI server needed): |
|
LGTM |
5 participants
