8334305: Remove all code for nsk.share.Log verbose mode #21267

rsunderbabu · 2024-09-30T14:55:33Z

Cleaning up nsk.share.Log code after the verbose mode was set always true.

Tested all the vmTestbase/ tests.

Progress

Change must be properly reviewed (1 review required, with at least 1 Reviewer)
Change must not contain extraneous whitespace
Commit message must refer to an issue

Issue

JDK-8334305: Remove all code for nsk.share.Log verbose mode (Enhancement - P4)

Reviewers

Leonid Mesnik (@lmesnik - Reviewer) Review applies to a346de1c
Hamlin Li (@Hamlin-Li - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/21267/head:pull/21267
$ git checkout pull/21267

Update a local copy of the PR:
$ git checkout pull/21267
$ git pull https://git.openjdk.org/jdk.git pull/21267/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 21267

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/21267.diff

Webrev

Link to Webrev Comment

bridgekeeper · 2024-09-30T14:56:34Z

👋 Welcome back rsunderbabu! 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 · 2024-09-30T14:58:18Z

@rsunderbabu 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:

8334305: Remove all code for  nsk.share.Log verbose mode

Reviewed-by: mli, lmesnik

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 25 new commits pushed to the master branch:

1b46fea: 8341024: [test] build/AbsPathsInImage.java fails with OOM when using ubsan-enabled binaries
9bd4785: 8340437: Open source few more AWT Frame related tests
988f13a: 8340354: Open source AWT desktop properties and print related tests
b11066b: 8340719: Open source AWT List tests
a32c3b4: 8340407: Open source a few more Component related tests
31858fc: 8340491: Thread stack-base assertion should report which thread has the un-set stack
bfdeb33: 8340332: Open source mixed AWT tests - Set3
a6b3188: 8337632: AES-GCM Algorithm optimization for x86_64
5586f83: 8341064: Define anchor point and index term for "wrapper classes"
4168faf: 8341100: Add index entries for terms used in java.lang.Class
... and 15 more: https://git.openjdk.org/jdk/compare/73ebb848fdb66861e912ea747c039ddd1f7a5f48...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.

As you do not have Committer status in this project an existing Committer must agree to sponsor your change. Possible candidates are the reviewers of this PR (@lmesnik, @Hamlin-Li) but any other Committer may sponsor as well.

➡️ To flag this PR as ready for integration with the above commit message, type /integrate in a new comment. (Afterwards, your sponsor types /sponsor in a new comment to perform the integration).

openjdk · 2024-09-30T14:59:21Z

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

hotspot
serviceability

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.

mlbridge · 2024-09-30T15:01:45Z

Webrevs

plummercj · 2024-09-30T21:07:23Z

test/hotspot/jtreg/vmTestbase/nsk/share/Log.java

@@ -315,16 +262,9 @@ public void trace(int level, Object message, Throwable exception) {

    /**
     * Print <code>message</code> to the assigned output stream,


Need replace the comma with a period.

plummercj · 2024-09-30T21:07:59Z

test/hotspot/jtreg/vmTestbase/nsk/share/Log.java

-        if (!verbose()) {
-            doPrint(message);
-        }


Is this method ever called? Is there a CR to remove it (and any references to it)?

These are deprecated methods and are still getting called. The calls need to be replaced with display. I will create a separate CR for this and address because the impact radius of such a change is bigger than this CR.

plummercj · 2024-09-30T21:10:02Z

test/hotspot/jtreg/vmTestbase/nsk/share/Log.java

@@ -406,10 +337,9 @@ private void logExceptionForFailureAnalysis(String msg) {
    /////////////////////////////////////////////////////////////////

    /**
-     * Redirect log to the given <code>stream</code>, and switch
-     * log mode to verbose.
+     * Redirect log to the given <code>stream</code>


Need period at end of sentence.

plummercj · 2024-09-30T21:10:57Z

test/hotspot/jtreg/vmTestbase/nsk/share/Log.java

     * Prints errors summary to current stream, cancel current stream
-     * and switches to new stream. Turns on verbose mode for new stream.
+     * and switches to new stream.


Does it really do all this? It looks to me like it just switches to the new stream. I'm not sure what is meant by "error summary" and cancelling.

marked for deprecation.

Ok, but unless it is going to be removed by another CR soon, I think the comments should at least reflect what it currently does.

rsunderbabu · 2024-10-01T01:14:22Z

/integrate

openjdk · 2024-10-01T01:15:39Z

@rsunderbabu
Your change (at version a346de1) is now ready to be sponsored by a Committer.

dholmes-ora · 2024-10-01T01:42:09Z

@rsunderbabu this should wait for the second review by @plummercj to be completed before it is integrated. Thanks.

rsunderbabu · 2024-10-01T01:49:02Z

@rsunderbabu this should wait for the second review by @plummercj to be completed before it is integrated. Thanks.

Thanks for pointing out David. I missed seeing those comments. I have addressed them now.

plummercj · 2024-10-01T04:02:39Z

test/hotspot/jtreg/vmTestbase/nsk/share/Log.java

 import java.util.HashMap;
 import java.util.Map;
 import java.util.Set;
 import java.util.HashSet;
 import java.util.Vector;

-import nsk.share.test.LazyFormatString;

 /**
 * This class helps to print test-execution trace messages


Missing period.

plummercj · 2024-10-01T04:03:25Z

test/hotspot/jtreg/vmTestbase/nsk/share/Log.java

-        if (!verbose()) {
-            doPrint(message);
-        }


plummercj · 2024-10-01T04:04:43Z

test/hotspot/jtreg/vmTestbase/nsk/share/Log.java

     * Prints errors summary to current stream, cancel current stream
-     * and switches to new stream. Turns on verbose mode for new stream.
+     * and switches to new stream.


Ok, but unless it is going to be removed by another CR soon, I think the comments should at least reflect what it currently does.

rsunderbabu · 2024-10-01T06:08:01Z

JDK-8341302 created for addressing deprecated method usage.

8334305: Remove all code for nsk.share.Log verbose mode

a346de1

openjdk bot changed the title ~~8334305: Remove all code for nsk.share.Log verbose mode~~ 8334305: Remove all code for nsk.share.Log verbose mode Sep 30, 2024

openjdk bot added the rfr Pull request is ready for review label Sep 30, 2024

openjdk bot added serviceability [email protected] hotspot [email protected] labels Sep 30, 2024

lmesnik approved these changes Sep 30, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Sep 30, 2024

plummercj reviewed Sep 30, 2024

View reviewed changes

openjdk bot added the sponsor Pull request is ready to be sponsored label Oct 1, 2024

openjdk bot removed sponsor Pull request is ready to be sponsored ready Pull request is ready to be integrated labels Oct 1, 2024

Addressed review comments

097789b

plummercj reviewed Oct 1, 2024

View reviewed changes

Addressed review comments part2

b895705

Hamlin-Li approved these changes Oct 1, 2024

View reviewed changes

openjdk bot added the ready Pull request is ready to be integrated label Oct 1, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

8334305: Remove all code for nsk.share.Log verbose mode #21267

8334305: Remove all code for nsk.share.Log verbose mode #21267

rsunderbabu commented Sep 30, 2024 •

edited by openjdk bot

Loading

bridgekeeper bot commented Sep 30, 2024

openjdk bot commented Sep 30, 2024 •

edited

Loading

openjdk bot commented Sep 30, 2024

mlbridge bot commented Sep 30, 2024 •

edited

Loading

plummercj Sep 30, 2024

plummercj Sep 30, 2024

rsunderbabu Oct 1, 2024

plummercj Oct 1, 2024

plummercj Sep 30, 2024

plummercj Sep 30, 2024

rsunderbabu Oct 1, 2024

plummercj Oct 1, 2024

rsunderbabu commented Oct 1, 2024

openjdk bot commented Oct 1, 2024

dholmes-ora commented Oct 1, 2024

rsunderbabu commented Oct 1, 2024

plummercj Oct 1, 2024

plummercj Oct 1, 2024

plummercj Oct 1, 2024

rsunderbabu commented Oct 1, 2024

		@@ -315,16 +262,9 @@ public void trace(int level, Object message, Throwable exception) {

		/**
		* Print <code>message</code> to the assigned output stream,

8334305: Remove all code for nsk.share.Log verbose mode #21267

Are you sure you want to change the base?

8334305: Remove all code for nsk.share.Log verbose mode #21267

Conversation

rsunderbabu commented Sep 30, 2024 • edited by openjdk bot Loading

Progress

Issue

Reviewers

Reviewing

Webrev

bridgekeeper bot commented Sep 30, 2024

openjdk bot commented Sep 30, 2024 • edited Loading

openjdk bot commented Sep 30, 2024

mlbridge bot commented Sep 30, 2024 • edited Loading

Webrevs

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rsunderbabu commented Oct 1, 2024

openjdk bot commented Oct 1, 2024

dholmes-ora commented Oct 1, 2024

rsunderbabu commented Oct 1, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rsunderbabu commented Oct 1, 2024

rsunderbabu commented Sep 30, 2024 •

edited by openjdk bot

Loading

openjdk bot commented Sep 30, 2024 •

edited

Loading

mlbridge bot commented Sep 30, 2024 •

edited

Loading