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:
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'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:
[email protected]
With regards,
Apache Git Services
