sijie commented on a change in pull request #1228: Issue #570: Move logic of
unpersistedbytes to bufferedchannel
URL: https://github.com/apache/bookkeeper/pull/1228#discussion_r172319905
##########
File path:
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BufferedChannel.java
##########
@@ -110,11 +150,15 @@ public long getFileChannelPosition() {
* @throws IOException if the write or sync operation fails.
*/
public void flush(boolean shouldForceWrite) throws IOException {
+ flush(shouldForceWrite, false);
+ }
+
+ public void flush(boolean shouldForceWrite, boolean forceMetadata) throws
IOException {
synchronized (this) {
Review comment:
well, I don't think it is code smell. It is okay to me, because the flags
define verify clear behavior: flush the buffer and if forceWrite enable, force
the channel with 'forceMetadata' flag. The method follows the java standard:
https://docs.oracle.com/javase/7/docs/api/java/nio/channels/FileChannel.html#force(boolean)
so going back to your comment here:
if your -1 is about the flag, `forceMetadata` flag is following the standard
:
https://docs.oracle.com/javase/7/docs/api/java/nio/channels/FileChannel.html#force(boolean)
if your -1 is about to ask Charan to break the methods into two methods, as
Charan explained and I pointed out, this breakdown is a "code refactor" which
will touch all the methods using `flush`. It is unrelated to the change, which
can be done in a separate PR, so it doesn't sound to be a blocker to the change
here.
so which part are you really giving -1 to?
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
