Skip to content

[BEAM-12160] Add TODO for fixing the warning - #14516

Merged
aromanenko-dev merged 1 commit into
apache:masterfrom
boyuanzz:build
Apr 15, 2021
Merged

[BEAM-12160] Add TODO for fixing the warning#14516
aromanenko-dev merged 1 commit into
apache:masterfrom
boyuanzz:build

Conversation

@boyuanzz

@boyuanzzboyuanzz commented Apr 13, 2021

Copy link
Copy Markdown
Contributor

Please add a meaningful description for your change here

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)

LangSDKULRDataflowFlinkSamzaSparkTwister2
GoBuild Status---Build StatusBuild Status---Build Status---
JavaBuild StatusBuild StatusBuild Status
Build Status
Build Status
Build Status
Build StatusBuild Status
Build Status
Build Status
Build Status
Build Status
Build Status
Build Status		Build StatusBuild Status
Build Status
Build Status		Build Status
PythonBuild Status
Build Status
Build Status		---Build Status
Build Status
Build Status		Build Status
Build Status		---Build Status---
XLangBuild Status---Build StatusBuild Status---Build Status---

Pre-Commit Tests Status (on master branch)

---JavaPythonGoWebsiteWhitespaceTypescript
Non-portableBuild Status
Build Status
Build Status
Build Status
Build Status		Build StatusBuild StatusBuild StatusBuild Status
Portable---Build StatusBuild 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.

@boyuanzz

boyuanzz commented Apr 13, 2021

Copy link
Copy Markdown
ContributorAuthor

r: @kennknowles

@boyuanzzboyuanzz mentioned this pull request Apr 13, 2021
Merged
4 tasks
@boyuanzz

boyuanzz commented Apr 13, 2021

Copy link
Copy Markdown
ContributorAuthor

Run Java PreCommit

@kennknowles

kennknowles commented Apr 13, 2021

Copy link
Copy Markdown
Member

I would like to merge this with multiple commits and not flattening them. I would like the formatting change to be a separate commit from the others. It is OK if it come later in topological order, but would you mind organizing the PR into commits that can be merged without squashing?

@kennknowles

kennknowles commented Apr 13, 2021

Copy link
Copy Markdown
Member

(mislick)

@boyuanzz

boyuanzz commented Apr 13, 2021

Copy link
Copy Markdown
ContributorAuthor

I would like to merge this with multiple commits and not flattening them. I would like the formatting change to be a separate commit from the others. It is OK if it come later in topological order, but would you mind organizing the PR into commits that can be merged without squashing?

There are 3 commits in this PR, where the last commit is the format changes.

@aromanenko-dev

aromanenko-dev commented Apr 13, 2021•
edited
Loading

Copy link
Copy Markdown
Contributor

It looks that this PR contradicts with another one #14373, that was submitted a while ago and was waiting for review. Additionally to unifying a build part for the module, it allows to run TPC-DS benchmark with SparkRunner as well.

Can we merge #14373 before and then adjust this PR accordingly?
CC: @iemejia@kennknowles

@iemejia

iemejia commented Apr 13, 2021•
edited
Loading

Copy link
Copy Markdown
Member

I merged Alexey's PR before because he opened the PR first and I had almost finished reviewing it.
Sorry for the inconvenience @boyuanzz. I have the impression both PRs intend was similar but Alexey also added the support for Spark runner and had the separated formatting commit. Can you please rebase if there are still missing changes now on top of master. Thanks.

@aromanenko-dev

aromanenko-dev commented Apr 13, 2021

Copy link
Copy Markdown
Contributor

To avoid the similar situations in the future, I'd like to suggest to sync for ongoing and future work for this module here or on dev@.

In the same time, I added a new component testing-tpcds in Jira (like we have for Nexmark) and encourage all to create new Jiras with linking to this component.

@kennknowles

kennknowles commented Apr 13, 2021

Copy link
Copy Markdown
Member

The conflicts should be no problem - just keep the version from master when rebasing and run spotless again.

@kennknowles

kennknowles commented Apr 13, 2021

Copy link
Copy Markdown
Member

I had thought from the change description "Address warning" that the middle commit should be squashed. But I can see that it should perhaps be topologically before using applyJavaNature.

@boyuanzz

boyuanzz commented Apr 13, 2021

Copy link
Copy Markdown
ContributorAuthor

Thanks for mentioning the existing changes, Ismael and Alexey. Let me rebase and to see whether we still have additional changes.

boyuanzz
boyuanzz commented Apr 13, 2021
View reviewed changes
Comment threadsdks/java/testing/tpcds/build.gradle Outdated

@boyuanzzboyuanzzApr 13, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

@iemejia@aromanenko-dev Do we want to publish this package during release?

@aromanenko-devaromanenko-devApr 13, 2021•
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Hmm, good catch. We do this for Nexmark but I don't think it's needed as published artifacts.
@iemejia wdyt?

@iemejiaiemejiaApr 14, 2021

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think publishing them is better to enable other parties to reproduce it outside of Beam. Also adding an extra module probably does not add too much extra weight to the release process.

@boyuanzzboyuanzzApr 14, 2021

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

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

Ack. So the only difference is I added 2 TODO to indicate addressing warngings.

@boyuanzz

boyuanzz commented Apr 13, 2021

Copy link
Copy Markdown
ContributorAuthor

Run Java PreCommit

@github-actions

github-actionsBot commented Apr 13, 2021

Copy link
Copy Markdown
Contributor

The Workflow run is cancelling this PR. It is an earlier duplicate of 2173354 run.

@boyuanzzboyuanzz changed the title [BEAM-12160] Use applyJavaNature in tcpds module[BEAM-12160] Add TODO for fixing the warningApr 14, 2021
@boyuanzz

boyuanzz commented Apr 14, 2021

Copy link
Copy Markdown
ContributorAuthor

Run Java PreCommit

3 similar comments
@aromanenko-dev

aromanenko-dev commented Apr 15, 2021

Copy link
Copy Markdown
Contributor

Run Java PreCommit

@boyuanzz

boyuanzz commented Apr 15, 2021

Copy link
Copy Markdown
ContributorAuthor

Run Java PreCommit

@boyuanzz

boyuanzz commented Apr 15, 2021

Copy link
Copy Markdown
ContributorAuthor

Run Java PreCommit

@aromanenko-dev
aromanenko-dev merged commit ebe68fb into apache:masterApr 15, 2021
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@iemejiaiemejiaiemejia left review comments
@aromanenko-devaromanenko-devaromanenko-dev left review comments
@kennknowleskennknowleskennknowles approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@boyuanzz@kennknowles@aromanenko-dev@iemejia