[BEAM-2057] Add a test for metrics reporting in Spark#2730
[BEAM-2057] Add a test for metrics reporting in Spark#2730holdenk wants to merge 2 commits intoapache:masterfrom
Conversation
|
Stupid Travis still being broken. (That's a bug for a larger hackathon :p) Overview: This looks pretty much perfect as-is -- great, though I'd probably defer review to @aviemzur who implemented the Spark metrics connector. However, I wonder is this test needed given that the |
|
Manual build started: https://builds.apache.org/view/Beam/job/beam_PreCommit_Java_MavenInstall/10086/ |
| public ExternalResource inMemoryMetricsSink = new InMemoryMetricsSinkRule(); | ||
|
|
||
| @Rule | ||
| public InMemoryMetricsSinkRule clearInMemoryMetricsSink = new InMemoryMetricsSinkRule(); |
There was a problem hiding this comment.
I believe either of the two will suffice, and the other one can be safely removed.
In NamedAggregatorsTest.java we had two rules like so:
@Rule
public ExternalResource inMemoryMetricsSink = new InMemoryMetricsSinkRule();
@Rule
public ClearAggregatorsRule clearAggregators = new ClearAggregatorsRule();due to the aggregators that needed to be cleared in addition to the metrics themselves.
The new Metrics API removes the use of aggregators, so this should not be an issue.
|
@dhalperi this is needed to test that Beam metrics are reported via Spark's metrics sink, this isn't tested by |
| /** | ||
| * A test for the metrics reporting in Spark. | ||
| */ | ||
| public class SparkMetricsTest { |
| public void testNonExistingMetricName() throws Exception { | ||
| runPipeline(); | ||
|
|
||
| final Long valueOf = InMemoryMetrics.<Long>valueOf("myMissingAggregator"); |
There was a problem hiding this comment.
Probably not, I'll remove this.
aviemzur
left a comment
aviemzur
left a comment
There was a problem hiding this comment.
Looks good! a few comments on top of what was already commented by others.
|
R: @aviemzur will handle pushing across the finish line, I shouldn't have interjected. Sorry :) |
s/SparkMetricsTest/SparkMetricsSinkTest/
holdenk
force-pushed
the
BEAM-2057-test-metrics-spark-metrics-sink
branch
from
b6d3f03 to
d498d84
Compare
|
Thanks y'all - great experience for my first Beam PR :) <3 |
5 participants
Add a test for metrics reporting in Spark