On Fri, 1 Dec 2023 09:48:23 GMT, Jaikiran Pai <j...@openjdk.org> wrote:
> Can I please get a review for this change to the test library's > `OutputAnalyzer` class, which proposes to remove some unnecessary logging > from the `getExitValue()` call? > > As noted in https://bugs.openjdk.org/browse/JDK-8321163, right now this > method logs: > > > [2023-11-24T11:47:54.557561Z] Waiting for completion for process 24909 > [2023-11-24T11:47:54.557873Z] Waiting for completion finished for process > 24909 > > > even when the process has already completed and the exit value already known. > The change in this PR makes it such that if the exit value is available then > we no longer log this (nor call `process.waitFor()`). > > No new tests have been added given the nature of this change. tier1, tier2 > and tier3 tests continue to pass with this change. Great find and nice to get rid of the extra logging! test/lib/jdk/test/lib/process/OutputBuffer.java line 150: > 148: @Override > 149: public int getExitValue() { > 150: Integer exitCode = this.processExitCode; Do we really need the local `exitCode` variable? Even if another multiple threads write to processExitCode, I expect them to all write a non-null Integer. test/lib/jdk/test/lib/process/OutputBuffer.java line 158: > 156: boolean aborted = true; > 157: try { > 158: this.processExitCode = exitCode = p.waitFor(); According to the `waitFor` javadocs it returns the "exit value" and this function is named `getExitValue()`. I propose that we rename `processExitCode` to `exitValue` (alt. `processExitValue`). ------------- PR Review: https://git.openjdk.org/jdk/pull/16919#pullrequestreview-1759560718 PR Review Comment: https://git.openjdk.org/jdk/pull/16919#discussion_r1411946205 PR Review Comment: https://git.openjdk.org/jdk/pull/16919#discussion_r1411948365
