Skip to content

Conversation

@pulasthi
Copy link
Contributor

@pulasthi pulasthi commented Aug 30, 2020

Adding changes to complete quickstart for Twister2 runner and adding runner maven archetypes


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.
  • Update CHANGES.md with noteworthy changes.
  • 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 Dataflow Flink Samza Spark Twister2
Go Build Status --- Build Status --- Build Status ---
Java Build Status Build Status
Build Status
Build Status
Build Status
Build Status
Build Status
Build Status Build Status
Build Status
Build Status
Build Status
Python Build Status
Build Status
Build Status
Build Status
Build Status
Build Status
Build Status
Build Status
Build Status
Build Status
--- Build Status ---
XLang Build Status --- Build Status --- Build Status ---

Pre-Commit Tests Status (on master branch)

--- Java Python Go Website Whitespace
Non-portable Build Status Build Status
Build Status
Build Status
Build Status
Build Status Build Status Build Status
Portable --- Build Status --- --- ---

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

GitHub Actions Tests Status (on master branch)

Build python source distribution and wheels
Python tests
Java tests

See CI.md for more information about GitHub Actions CI.

@pulasthi
Copy link
Contributor Author

R: @iemejia

@pulasthi
Copy link
Contributor Author

@iemejia Sorry about the delay in getting this out. I am not sure what triggers I need to call in order to validate these changes, would you be able to call them or point them out. Thanks in advance

@codecov
Copy link

codecov bot commented Aug 30, 2020

Codecov Report

Merging #12731 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master   #12731   +/-   ##
=======================================
  Coverage   40.22%   40.22%           
=======================================
  Files         454      454           
  Lines       53669    53669           
=======================================
  Hits        21587    21587           
  Misses      32082    32082           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1d25e2e...8339055. Read the comment docs.

@aaltay aaltay requested a review from iemejia September 11, 2020 00:54
@iemejia
Copy link
Member

iemejia commented Sep 14, 2020

Run Twister2 ValidatesRunner

Copy link
Member

@iemejia iemejia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM Thanks @pulasthi good one to promote use of the runner on new users and validation on releases.
Since the next release will include this please stay tuned in case the release manager may have any issue with the extra validation step (hopefully not).

import org.apache.beam.sdk.PipelineResult;
import org.apache.beam.sdk.metrics.MetricResults;
import org.joda.time.Duration;
import org.mortbay.log.Log;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any particular reason to use a different logger kind here?
We use slf4j in most of the Beam code base for logging so maybe good to follow this on the Twister2 runner. Notice that this can be done in a separate PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@iemejia No, I will create separate PR that encapsulates all the changes you have mentioned. I will remove this log import

</dependencies>
</profile>
<profile>
<id>twister2-runner</id>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍


/** Check if the Runner is set to use Twister local mode or pointing to a deployment. */
private boolean isLocalMode(Twister2PipelineOptions options) {
if (options.getTwister2Home() == null || "".equals(options.getTwister2Home())) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

with guava (use vendor if so) this can be simpler, but that's not blocking at all for this PR
return Strings.isNullOrEmpty(options.getTwister2Home());

@iemejia iemejia merged commit f2dcbbe into apache:master Sep 14, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants