[BEAM-9205] Add MetricsPusherTest to validates runner tests and fix spark module VR tests configuration #10669

echauchot · 2020-01-23T14:09:32Z

I figured out that this test is no more run, So added it
I also cleaned another uneeded test that was incorrectly copy-pasted from the current spark runner (that uses a dedicated streaming test mechanism) to the new one

Thank you for your contribution! Follow this checklist to help us incorporate your contribution quickly and easily:

Choose reviewer(s) and mention them in a comment (R: @username).
Format the pull request title like [BEAM-XXX] Fixes bug in ApproximateQuantiles, where you replace BEAM-XXX with the appropriate JIRA issue, if applicable. This will automatically link the pull request to the issue.
If this contribution is large, please file an Apache Individual Contributor License Agreement.

See the Contributor Guide for more tips on how to make review process smoother.

Post-Commit Tests Status (on master branch)

Lang	SDK	Apex	Dataflow	Gearpump	Samza	Spark
Go		---	---	---	---
Java
Python		---		---	---
XLang	---	---	---	---	---	---

Pre-Commit Tests Status (on master branch)

---	Java	Python	Go	Website
Non-portable
Portable	---		---	---

See .test-infra/jenkins/README for trigger phrase, status and link of all Jenkins jobs.

echauchot · 2020-01-23T14:29:03Z

Run Spark ValidatesRunner

echauchot · 2020-01-23T14:29:14Z

Run Spark StructuredStreaming ValidatesRunner

iemejia · 2020-01-23T14:34:15Z

Run Flink ValidatesRunner

iemejia · 2020-01-23T14:34:42Z

Run Java Spark PortableValidatesRunner Batch

iemejia · 2020-01-23T14:35:31Z

Run Java Flink PortableValidatesRunner Batch

iemejia · 2020-01-23T14:35:40Z

Run Java Flink PortableValidatesRunner Streaming

echauchot · 2020-01-23T15:34:14Z

Run Java PreCommit

iemejia

Changes look good, but It seems that MetricsPusherTest is not being run any of the Spark runners, can you please check why @echauchot.

See:
https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_Spark_PR/203/testReport/
https://builds.apache.org/job/beam_PostCommit_Java_ValidatesRunner_SparkStructuredStreaming_PR/22/testReport/

For Flink they are running (and passing):
https://builds.apache.org/job/beam_PostCommit_Java_PVR_Flink_Batch_PR/119/testReport/org.apache.beam.runners.core.metrics/

iemejia · 2020-01-23T16:44:08Z

...c/test/java/org/apache/beam/runners/spark/structuredstreaming/metrics/MetricsPusherTest.java

- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-package org.apache.beam.runners.spark.structuredstreaming.metrics;


👏 removing useless code always good

echauchot · 2020-01-24T10:08:52Z

thanks @iemejia I'll take a look but I have an idea why. I think VR tests conf has changed, and as this MetricsPusherTest is the only one in runner-core package....
This is because it tests a runner feature (ability to push mertrics) not the compliance of the runner to the sdk specs.

echauchot · 2020-01-24T10:22:31Z

I think the VR conf of both spark runners is messed up, I will sync with other runners

echauchot · 2020-01-24T11:04:20Z

Run Spark StructuredStreaming ValidatesRunner

echauchot · 2020-01-24T11:04:44Z

Run Java Spark PortableValidatesRunner Batch

echauchot · 2020-01-24T11:05:04Z

Run Spark ValidatesRunner

echauchot · 2020-01-24T13:56:01Z

Run Java PreCommit

echauchot · 2020-01-24T14:01:52Z

@iemejia now validates runner tests are ok and MetricsPusher is run and passes for all spark the runners that support metrics (current and structured streaming ones).
PTAL.

Also the build seems flaky I opened a ticket: https://jira.apache.org/jira/browse/BEAM-9187 and relaunched the build

iemejia · 2020-01-24T15:23:36Z

Looks like the test is not run yet in the Portable Spark runner, but the Portable Flink runner did run it.

echauchot · 2020-01-27T09:08:15Z

Looks like the test is not run yet in the Portable Spark runner, but the Portable Flink runner did run it.

Yes, portable spark runner currently excludes all the metrics related tests that is why this test is not run. I guess the portable runner is not ready for metrics yet

iemejia · 2020-01-27T13:42:29Z

Interesting this was supposed to be working already. I will look at that with Kyle. I just looked for UsesMetricsPusher and did not see the exclusion that's why I expected the tests to run.

iemejia

LGTM Please self-merge / squash commits as you consider correct. Maybe worth to add A JIRA ticket and associate it with the previous ticket BEAM-3310.

iemejia · 2020-01-27T17:01:44Z

Just for ref I addressed the missing MetricsPusherTest on Portable Spark Runner on #10693

…lass (not only on the methods) and remove unneeded MetricsPusherTest inside spark structured streaming runner (because this runner uses the VR common test)

echauchot · 2020-01-28T10:43:42Z

Thanks @iemejia ! I have squashed and I'm merging on green light. I also have created and linked the ticket. And nice for https://github.com/apache/beam/pull/10693 !

echauchot · 2020-01-28T12:20:25Z

Run Portable_Python PreCommit

echauchot requested a review from iemejia January 23, 2020 14:09

iemejia requested changes Jan 23, 2020

View reviewed changes

iemejia approved these changes Jan 27, 2020

View reviewed changes

echauchot force-pushed the metricsPusher_validates_runner branch from 5170bd1 to 36c6b3a Compare January 28, 2020 10:33

echauchot added 2 commits January 28, 2020 11:39

[BEAM-9205] Add ValidatesRunner annotation to the MetricsPusherTest c…

c7914bd

…lass (not only on the methods) and remove unneeded MetricsPusherTest inside spark structured streaming runner (because this runner uses the VR common test)

[BEAM-9205] Fix validatesRunner tests configuration in spark module

bc9243a

echauchot force-pushed the metricsPusher_validates_runner branch from 36c6b3a to bc9243a Compare January 28, 2020 10:40

echauchot changed the title ~~Add MetricsPusherTest to validates runner tests~~ [BEAM-9205] Add MetricsPusherTest to validates runner tests and fix spark module VR tests configuration Jan 29, 2020

echauchot merged commit 5257ed3 into apache:master Jan 29, 2020

echauchot deleted the metricsPusher_validates_runner branch February 3, 2020 13:50

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[BEAM-9205] Add MetricsPusherTest to validates runner tests and fix spark module VR tests configuration #10669

[BEAM-9205] Add MetricsPusherTest to validates runner tests and fix spark module VR tests configuration #10669

echauchot commented Jan 23, 2020 •

edited

Loading

echauchot commented Jan 23, 2020

echauchot commented Jan 23, 2020

iemejia commented Jan 23, 2020

iemejia commented Jan 23, 2020

iemejia commented Jan 23, 2020

iemejia commented Jan 23, 2020

echauchot commented Jan 23, 2020

iemejia left a comment •

edited

Loading

iemejia Jan 23, 2020

echauchot Jan 24, 2020

echauchot commented Jan 24, 2020 •

edited

Loading

echauchot commented Jan 24, 2020 •

edited

Loading

echauchot commented Jan 24, 2020

echauchot commented Jan 24, 2020

echauchot commented Jan 24, 2020

echauchot commented Jan 24, 2020

echauchot commented Jan 24, 2020 •

edited

Loading

iemejia commented Jan 24, 2020

echauchot commented Jan 27, 2020

iemejia commented Jan 27, 2020

iemejia left a comment

iemejia commented Jan 27, 2020

echauchot commented Jan 28, 2020

echauchot commented Jan 28, 2020

[BEAM-9205] Add MetricsPusherTest to validates runner tests and fix spark module VR tests configuration #10669

[BEAM-9205] Add MetricsPusherTest to validates runner tests and fix spark module VR tests configuration #10669

Conversation

echauchot commented Jan 23, 2020 • edited Loading

I figured out that this test is no more run, So added it I also cleaned another uneeded test that was incorrectly copy-pasted from the current spark runner (that uses a dedicated streaming test mechanism) to the new one

Post-Commit Tests Status (on master branch)

Pre-Commit Tests Status (on master branch)

echauchot commented Jan 23, 2020

echauchot commented Jan 23, 2020

iemejia commented Jan 23, 2020

iemejia commented Jan 23, 2020

iemejia commented Jan 23, 2020

iemejia commented Jan 23, 2020

echauchot commented Jan 23, 2020

iemejia left a comment • edited Loading

Choose a reason for hiding this comment

iemejia Jan 23, 2020

Choose a reason for hiding this comment

echauchot Jan 24, 2020

Choose a reason for hiding this comment

echauchot commented Jan 24, 2020 • edited Loading

echauchot commented Jan 24, 2020 • edited Loading

echauchot commented Jan 24, 2020

echauchot commented Jan 24, 2020

echauchot commented Jan 24, 2020

echauchot commented Jan 24, 2020

echauchot commented Jan 24, 2020 • edited Loading

iemejia commented Jan 24, 2020

echauchot commented Jan 27, 2020

iemejia commented Jan 27, 2020

iemejia left a comment

Choose a reason for hiding this comment

iemejia commented Jan 27, 2020

echauchot commented Jan 28, 2020

echauchot commented Jan 28, 2020

echauchot commented Jan 23, 2020 •

edited

Loading

I figured out that this test is no more run, So added it
I also cleaned another uneeded test that was incorrectly copy-pasted from the current spark runner (that uses a dedicated streaming test mechanism) to the new one

iemejia left a comment •

edited

Loading

echauchot commented Jan 24, 2020 •

edited

Loading

echauchot commented Jan 24, 2020 •

edited

Loading

echauchot commented Jan 24, 2020 •

edited

Loading