Re: [PR] fix: resource leak when JournalChannel is not fully initialized [bookkeeper]
eolivelli merged PR #4340: URL: https://github.com/apache/bookkeeper/pull/4340 -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@bookkeeper.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] fix: resource leak when JournalChannel is not fully initialized [bookkeeper]
shoothzj commented on PR #4340: URL: https://github.com/apache/bookkeeper/pull/4340#issuecomment-2101856802 @dlg99 @eolivelli @hangc0276 @merlimat @nicoloboschi @StevenLuMT @wenbingshen @zhaijack PTAL -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@bookkeeper.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] fix: resource leak when JournalChannel is not fully initialized [bookkeeper]
shoothzj commented on PR #4340: URL: https://github.com/apache/bookkeeper/pull/4340#issuecomment-2098206949 @dlg99 @eolivelli @hangc0276 @merlimat @nicoloboschi @StevenLuMT @wenbingshen @zhaijack PTAL -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@bookkeeper.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] fix: resource leak when JournalChannel is not fully initialized [bookkeeper]
shoothzj commented on PR #4340: URL: https://github.com/apache/bookkeeper/pull/4340#issuecomment-2097197427 @dlg99 @eolivelli @hangc0276 @merlimat @nicoloboschi @StevenLuMT @wenbingshen @zhaijack PTAL -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@bookkeeper.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
Re: [PR] fix: resource leak when JournalChannel is not fully initialized [bookkeeper]
shoothzj commented on code in PR #4340:
URL: https://github.com/apache/bookkeeper/pull/4340#discussion_r1590666985
##
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/JournalChannel.java:
##
@@ -192,7 +192,6 @@ private JournalChannel(File journalDirectory, long logId,
writeHeader(bcBuilder, writeBufferSize);
} else { // open an existing file to read.
fc = channel.getFileChannel();
-bc = null; // readonly
Review Comment:
@eolivelli I think it's impossible unless we use Optional. And I think it
will not be better to use Optional here.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] fix: resource leak when JournalChannel is not fully initialized [bookkeeper]
eolivelli commented on code in PR #4340:
URL: https://github.com/apache/bookkeeper/pull/4340#discussion_r1590655146
##
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/JournalChannel.java:
##
@@ -192,7 +192,6 @@ private JournalChannel(File journalDirectory, long logId,
writeHeader(bcBuilder, writeBufferSize);
} else { // open an existing file to read.
fc = channel.getFileChannel();
-bc = null; // readonly
Review Comment:
can we make the fields "final" ?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] fix: resource leak when JournalChannel is not fully initialized [bookkeeper]
shoothzj commented on code in PR #4340:
URL: https://github.com/apache/bookkeeper/pull/4340#discussion_r1590554009
##
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/JournalChannel.java:
##
@@ -192,7 +192,6 @@ private JournalChannel(File journalDirectory, long logId,
writeHeader(bcBuilder, writeBufferSize);
} else { // open an existing file to read.
fc = channel.getFileChannel();
-bc = null; // readonly
Review Comment:
can we use annotation instead? I add annotation
```
// readonly, use fileChannel directly, no need to use BufferedChannel
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] fix: resource leak when JournalChannel is not fully initialized [bookkeeper]
shoothzj commented on code in PR #4340:
URL: https://github.com/apache/bookkeeper/pull/4340#discussion_r1590553534
##
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/JournalChannel.java:
##
@@ -299,6 +298,8 @@ int read(ByteBuffer dst)
public void close() throws IOException {
if (bc != null) {
bc.close();
+} else if (fc != null) {
Review Comment:
@eolivelli bc.close will close the ubder FileChannel, we can avoid one call
to `close()`.
BufferChannel will close the FileChannel
https://github.com/apache/bookkeeper/blob/b38b1717a7431d35f30439d2f47e4751be342357/bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BufferedChannel.java#L100-L105
FileChannel's close in jdk17
```
public final void close() throws IOException {
synchronized (closeLock) {
if (closed)
return;
closed = true;
implCloseChannel();
}
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] fix: resource leak when JournalChannel is not fully initialized [bookkeeper]
shoothzj commented on code in PR #4340:
URL: https://github.com/apache/bookkeeper/pull/4340#discussion_r1590554009
##
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/JournalChannel.java:
##
@@ -192,7 +192,6 @@ private JournalChannel(File journalDirectory, long logId,
writeHeader(bcBuilder, writeBufferSize);
} else { // open an existing file to read.
fc = channel.getFileChannel();
-bc = null; // readonly
Review Comment:
can we use annotation instead? like
// when readonly, we use filechannel directly without bufferchannel
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] fix: resource leak when JournalChannel is not fully initialized [bookkeeper]
shoothzj commented on code in PR #4340:
URL: https://github.com/apache/bookkeeper/pull/4340#discussion_r1590553534
##
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/JournalChannel.java:
##
@@ -299,6 +298,8 @@ int read(ByteBuffer dst)
public void close() throws IOException {
if (bc != null) {
bc.close();
+} else if (fc != null) {
Review Comment:
@eolivelli bc.close will close the ubder FileChannel, double close may raise
some other exception
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] fix: resource leak when JournalChannel is not fully initialized [bookkeeper]
eolivelli commented on code in PR #4340:
URL: https://github.com/apache/bookkeeper/pull/4340#discussion_r1590536703
##
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/JournalChannel.java:
##
@@ -299,6 +298,8 @@ int read(ByteBuffer dst)
public void close() throws IOException {
if (bc != null) {
bc.close();
+} else if (fc != null) {
Review Comment:
I would remove "else" and handle the 2 variables indipendenly
also we should try to close both of them in spite of any errors
```
try {
if (bc != null) {
bc.close();
}
} finally {
if (fc != null) {
fc.close();
}
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] fix: resource leak when JournalChannel is not fully initialized [bookkeeper]
eolivelli commented on code in PR #4340:
URL: https://github.com/apache/bookkeeper/pull/4340#discussion_r1590536094
##
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/JournalChannel.java:
##
@@ -192,7 +192,6 @@ private JournalChannel(File journalDirectory, long logId,
writeHeader(bcBuilder, writeBufferSize);
} else { // open an existing file to read.
fc = channel.getFileChannel();
-bc = null; // readonly
Review Comment:
why ermoving this line ?
in this code path writeHeader has not been called and bc is already null
setting it to null here is useful to understand
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscr...@bookkeeper.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
Re: [PR] fix: resource leak when JournalChannel is not fully initialized [bookkeeper]
shoothzj commented on PR #4340: URL: https://github.com/apache/bookkeeper/pull/4340#issuecomment-2094744842 @dlg99 @eolivelli @hangc0276 @merlimat @nicoloboschi @StevenLuMT @wenbingshen @zhaijack PTAL -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: commits-unsubscr...@bookkeeper.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
