ivankelly 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_r172443006

 File path: 
 @@ -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's code smell. Google "boolean parameter". You'll get a selection of hits 
explaining why they are bad, including a blog post from Martin Fowler and 
another from Robert C Martin who wrote the book on clean code.
   > because the flags define verify clear behavior
   Do they? In 6 months are you going to read flush(false, true) and know 
exactly what the flags are doing?
   > The method follows the java standard:
   A 16 year old API. It can hardly be held up as a gold standard. Still 
though, it's not forceMetadata in itself I'm strongly opposed to. Its adding 
*another* boolean flag, to a method that already has one, that I'm opposed to.
   > 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.
   There's no refactor involved here. ```flush(boolean,boolean)``` isn't even 
called, so if it's removed, you will only have update ```flush(boolean)```.

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:

With regards,
Apache Git Services

Reply via email to