Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove SuppressWarnings from NoopLock.java #27416

Merged

Conversation

damondouglas
Copy link
Contributor

@damondouglas damondouglas commented Jul 9, 2023

This PR addresses #20507 by removing the @SuppressWarnings annotation from sdks/java/core/src/main/java/org/apache/beam/sdk/util/NoopLock.java.


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

  • Mention the appropriate issue in your description (for example: addresses #123), if applicable. This will automatically add a link to the pull request in the issue. If you would like the issue to automatically close on merging the pull request, comment fixes #<ISSUE NUMBER> instead.
  • 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.

To check the build health, please visit https://github.com/apache/beam/blob/master/.test-infra/BUILD_STATUS.md

GitHub Actions Tests Status (on master branch)

Build python source distribution and wheels
Python tests
Java tests
Go tests

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

@github-actions github-actions bot added the java label Jul 9, 2023
@damondouglas damondouglas marked this pull request as ready for review July 9, 2023 19:48
@github-actions
Copy link
Contributor

github-actions bot commented Jul 9, 2023

Checks are failing. Will not request review until checks are succeeding. If you'd like to override that behavior, comment assign set of reviewers

@damondouglas
Copy link
Contributor Author

assign set of reviewers

@github-actions
Copy link
Contributor

github-actions bot commented Jul 9, 2023

Assigning reviewers. If you would like to opt out of this review, comment assign to next reviewer:

R: @bvolpato for label java.

Available commands:

  • stop reviewer notifications - opt out of the automated review tooling
  • remind me after tests pass - tag the comment author after tests pass
  • waiting on author - shift the attention set back to the author (any comment or push by the author will return the attention set to the reviewers)

The PR bot will only process comments in the main thread (not review comments).

@damondouglas
Copy link
Contributor Author

Run Java PreCommit

@damondouglas damondouglas requested a review from kennknowles July 11, 2023 04:07
@damondouglas
Copy link
Contributor Author

@kennknowles related to #27416 (comment) and #27416 (comment), I'm thinking we are going to run into a lot of these issues as we clean up a lot of the SupressWarnings. I'm wondering if we should just maintain a like-for-like as much as the original author's intent unless we observe something breaking. What do you think?

@kennknowles
Copy link
Member

To your global comment: actually the issue with the repo is that the classes that exist are wrong and the process of fixing the nullness discipline is to make them right. Sometimes that just involves putting on a few annotations. Sometimes it involves refactors or redesigns because the original code cannot be made right.

If we just wanted "like for like" then suppressing warnings on the places where we see warnings would do the trick. But then of course we still have all the same errors in the codebase.

@kennknowles
Copy link
Member

In other words, running into these issues and fixing them is the whole point.

public class NoopLock implements Lock, Serializable {

private static NoopLock instance;
private static @MonotonicNonNull NoopLock instance;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kennknowles I chose @MonotonicNonNull here as per the JavaDoc it "Indicates that once the field (or variable) becomes non-null, it never becomes null again." I preferred this over the original @Nullable as the static get method along with the private constructor satisfies this condition.


public static NoopLock get() {
public static @NonNull NoopLock get() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The @NonNull annotation seemed appropriate here given that the returned instance, if null, will always be instantiated.

@damondouglas damondouglas requested a review from kennknowles July 16, 2023 20:15
@kennknowles
Copy link
Member

run java precommit

@kennknowles
Copy link
Member

I didn't see why it was red but it also has be GC'd

@damondouglas damondouglas merged commit b610846 into apache:master Jul 20, 2023
@damondouglas damondouglas deleted the 20507-remove-suppresswarnings-NoopLock branch July 20, 2023 22:04
@kennknowles
Copy link
Member

The tests were not green. Please do not merge when tests are failing.

@kennknowles
Copy link
Member

Even though the failures at https://ci-beam.apache.org/job/beam_PreCommit_Java_Phrase/6182/ are probably not related, please file tickets about the failures and kick the tests to get them green. Merging on red embraces the flakey lifestyle, which we don't want to do.

cushon pushed a commit to cushon/beam that referenced this pull request May 24, 2024
* Remove SuppressWarnings from NoopLock.java

* Remove Nonnull annotations from original
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