On Tue, 2 Apr 2024 04:48:37 GMT, Alan Bateman <al...@openjdk.org> wrote:
>> Tim Prinzing has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Add support for AsynchronousFileChannel.force().
>
> src/java.base/share/classes/jdk/internal/event/FileForceEvent.java line 35:
>
>> 33: * {@link #commit(long, long, String, boolean)} method
>> 34: * must be the same as the order of the fields.
>> 35: */
>
> You may have to re-word this comment to avoid confusion with the metaData
> parameter. That is, there is event meta data and there is file metaData, if
> you see what I mean.
I see what you mean. I reworded the javadoc.
> test/jdk/jdk/jfr/event/io/TestAsynchronousFileChannelEvents.java line 50:
>
>> 48:
>> 49: public static void main(String[] args) throws Throwable {
>> 50: File blah = File.createTempFile("blah", null);
>
> Can you change this to use the tests's scratch rather that /tmp, meaning
> `Files.createTempFile(Path.of("."), "blah", "jfr")`. That way the recording
> is available in the event that the test fails.
I'm not sure what you mean about the recording. The file is the
AsynchronousFileChannel under test and does not contain the event recording.
> test/jdk/jdk/jfr/event/io/TestAsynchronousFileChannelEvents.java line 64:
>
>> 62:
>> 63: data.flip();
>> 64: ch.write(data, 0);
>
> This just initiates the write operation, it doesn't wait until it completes.
> It returns a Future so adding .get() will ensure that it waits and that there
> is potentially data to write back to the file system.
I do realize the write doesn't wait. I was under the impression that flush()
does wait until everything has been flushed to disk. I went ahead and added
.get() as requested.
-------------
PR Review Comment: https://git.openjdk.org/jdk/pull/18542#discussion_r1566405842
PR Review Comment: https://git.openjdk.org/jdk/pull/18542#discussion_r1566413683
PR Review Comment: https://git.openjdk.org/jdk/pull/18542#discussion_r1566409791
