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

8270859: Post JEP 411 refactoring: client libs with maximum covering > 10K #4815

Closed
wants to merge 2 commits into from

Conversation

wangweij
Copy link
Contributor

@wangweij wangweij commented Jul 16, 2021

This is the last part of Post JEP 411 refactoring that makes @SuppressWarnings("removal") more fine grained. This fix deals with all client libs annotations that cover more than 10K bytes of code.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8270859: Post JEP 411 refactoring: client libs with maximum covering > 10K

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/4815/head:pull/4815
$ git checkout pull/4815

Update a local copy of the PR:
$ git checkout pull/4815
$ git pull https://git.openjdk.java.net/jdk pull/4815/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4815

View PR using the GUI difftool:
$ git pr show -t 4815

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4815.diff

…> 10K

8270859: Post JEP 411 refactoring: client libs with maximum covering > 10K
@bridgekeeper
Copy link

bridgekeeper bot commented Jul 16, 2021

👋 Welcome back weijun! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot added the rfr Pull request is ready for review label Jul 16, 2021
@openjdk
Copy link

openjdk bot commented Jul 16, 2021

@wangweij The following labels will be automatically applied to this pull request:

  • 2d
  • awt
  • swing

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added 2d client-libs-dev@openjdk.org swing client-libs-dev@openjdk.org awt client-libs-dev@openjdk.org labels Jul 16, 2021
@mlbridge
Copy link

mlbridge bot commented Jul 16, 2021

Webrevs

} catch (Throwable e) {
// We don't care about component.
// So don't prevent class initialisation.
e.printStackTrace();
Copy link

@Vest Vest Jul 16, 2021

Choose a reason for hiding this comment

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

I am not the reviewer, but I am curious. If we do not care about the exception, is it a right way to just print it to the console as it is? If people use loggers, they won’t be able to capture this stack trace.
thank you for your answer in advance.
p.s. I know it is not your code, but isn’t it a suitable time to improve this part?

Copy link
Contributor

@prrace prrace Jul 17, 2021

Choose a reason for hiding this comment

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

No, that has nothing to do with what is being done here so it is a terrible time to do it.

Copy link
Member

@kevinrushforth kevinrushforth Jul 16, 2021

Choose a reason for hiding this comment

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

No. A refactoring change like this is the wrong time to make any unrelated changes. If such changes are a good idea, they should be done in a different bug with a separate pull request.

} catch (Throwable e) {
// We don't care about component.
// So don't prevent class initialisation.
e.printStackTrace();
Copy link
Contributor

@prrace prrace Jul 17, 2021

Choose a reason for hiding this comment

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

No, that has nothing to do with what is being done here so it is a terrible time to do it.

@@ -126,15 +127,15 @@
}
}

osname = java.security.AccessController.doPrivileged(
new sun.security.action.GetPropertyAction("os.name"));
osname = System.getProperty("os.name");
Copy link
Contributor

@prrace prrace Jul 17, 2021

Choose a reason for hiding this comment

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

I suppose that you are relying on the default security settings which allow access to this property ?
Do you have reason to believe that it is common to make this assumption in code in the JDK ?
Why take the chance ?

Copy link
Contributor Author

@wangweij wangweij Jul 19, 2021

Choose a reason for hiding this comment

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

I've seen it elsewhere. For example:

if (System.getProperty("os.name").equals("Linux")) {
and
osName = System.getProperty("os.name");
.

That said, I'll revert it because it's a behavior change.

Copy link
Contributor Author

@wangweij wangweij Jul 19, 2021

Choose a reason for hiding this comment

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

New commit pushed.

@mrserb
Copy link
Member

mrserb commented Jul 17, 2021

After the requested change to the PrintServiceLookupProvider.java. Please confirm that the client tests are green.

@wangweij
Copy link
Contributor Author

wangweij commented Jul 19, 2021

After the requested change to the PrintServiceLookupProvider.java. Please confirm that the client tests are green.

I ran all test from tier1 to tier9. There are 6 JCK testing failures in tier9. Each failed with "Problem communicating with agent" or "premature EOF from agent". The same tests (with the same VM option) also fail intermittently in other test runs recently. These failures should not be related to this code change.

mrserb
mrserb approved these changes Jul 21, 2021
Copy link
Member

@mrserb mrserb left a comment

Looks fine to me.

@openjdk
Copy link

openjdk bot commented Jul 21, 2021

@wangweij This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8270859: Post JEP 411 refactoring: client libs with maximum covering > 10K

Reviewed-by: serb

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 28 new commits pushed to the master branch:

  • cd8783c: 8270820: remove unused stiFileTableIndex from SDE.c
  • 1f51e13: 8270147: Increase stride size allowing unrolling more loops
  • 7dd19af: 8270803: Reduce CDS API verbosity
  • 6346793: 8269933: test/jdk/javax/net/ssl/compatibility/JdkInfo incorrect verification of protocol and cipher support
  • 1eeb179: Merge
  • 61359c4: 8270993: Missing forward declaration of ZeroFrame
  • 8e8e87a: 8268223: Problemlist vmTestbase/nsk/jdi/HiddenClass/events/events001.java
  • 845c31d: 8269240: java/foreign/stackwalk/TestAsyncStackWalk.java test failed with concurrent GC
  • 0cec11d: 8270307: C2: assert(false) failed: bad AD file after JDK-8267687
  • 38694aa: 8270939: ProblemList java/lang/invoke/RicochetTest.java until JDK-8251969 is fixed
  • ... and 18 more: https://git.openjdk.java.net/jdk/compare/1d8d72d2c24764ab32741dd5220a6c1a980656e2...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added the ready Pull request is ready to be integrated label Jul 21, 2021
@wangweij
Copy link
Contributor Author

wangweij commented Jul 27, 2021

/integrate

@openjdk
Copy link

openjdk bot commented Jul 27, 2021

Going to push as commit 90cd2fa.
Since your change was applied there have been 124 commits pushed to the master branch:

  • c8af823: 8267485: Remove the dependency on SecurityManager in JceSecurityManager.java
  • ea49691: 8270794: Avoid loading Klass* twice in TypeArrayKlass::oop_size()
  • fc80a6b: 8270946: X509CertImpl.getFingerprint should not return the empty String
  • 45d277f: 8270308: Arena::Amalloc may return misaligned address on 32-bit
  • fde1831: 8212961: [TESTBUG] vmTestbase/nsk/stress/jni/ native code cleanup
  • bb508e1: 8269753: Misplaced caret in PatternSyntaxException's detail message
  • c3d8e92: 8190753: (zipfs): Accessing a large entry (> 2^31 bytes) leads to a negative initial size for ByteArrayOutputStream
  • eb6da88: Merge
  • b76a838: 8269150: UnicodeReader not translating \u005c\u005d to \]
  • 7ddabbf: 8271175: runtime/jni/FindClassUtf8/FindClassUtf8.java doesn't have to be run in othervm
  • ... and 114 more: https://git.openjdk.java.net/jdk/compare/1d8d72d2c24764ab32741dd5220a6c1a980656e2...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Jul 27, 2021
@openjdk openjdk bot added integrated Pull request has been integrated and removed ready Pull request is ready to be integrated labels Jul 27, 2021
@openjdk
Copy link

openjdk bot commented Jul 27, 2021

@wangweij Pushed as commit 90cd2fa.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

@openjdk openjdk bot removed the rfr Pull request is ready for review label Jul 27, 2021
@wangweij wangweij deleted the 8270859 branch Jul 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2d client-libs-dev@openjdk.org awt client-libs-dev@openjdk.org integrated Pull request has been integrated swing client-libs-dev@openjdk.org
5 participants