[GitHub] ivankelly commented on a change in pull request #1228: Issue #570: Move logic of unpersistedbytes to bufferedchannel
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_r173113575 ## File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/LedgerManagerFactory.java ## @@ -94,7 +94,7 @@ LedgerUnderreplicationManager newLedgerUnderreplicationManager() * @param lm *Layout manager */ Review comment: Ah, weird. I wonder why checkstyle doesn't pick it up as part of the build. Anyhow, really shouldn't be in this patch, but will let it pass. It's the other issues I'm more concerned about. 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] ivankelly commented on a change in pull request #1228: Issue #570: Move logic of unpersistedbytes to bufferedchannel
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_r172496811
##
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:
> fix all the shortcomings of the existing code.
You are adding a new method. I'm asking you not to add to issues that
already exist.
> It is risky and burdensome to touch critical path (change signatures)
You don't have to change any signatures if you don't want to. Just call
flush(false), and then forceWrite(true). flush() was just a suggestion as the
false is redundant there. BufferedChannel should have a simple flush()
operation that just flushing the buffer to the channel, which is exactly what
flush(false) does. You don't have to add that though.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] ivankelly commented on a change in pull request #1228: Issue #570: Move logic of unpersistedbytes to bufferedchannel
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_r172463549
##
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:
refactor refers to existing code. The next subtask is not existing code, so
calls in it to ```flush(boolean, boolean)```, can be replaced with calls to
```flush(); forceWrite(boolean);``` or ```flushAndForceWrite(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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] ivankelly commented on a change in pull request #1228: Issue #570: Move logic of unpersistedbytes to bufferedchannel
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_r172447036
##
File path:
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BufferedChannel.java
##
@@ -138,6 +182,20 @@ public long forceWrite(boolean forceMetadata) throws
IOException {
// the force write, any flush that happens after this may or may
// not be flushed
long positionForceWrite = writeBufferStartPosition.get();
+/*
+ * since forceWrite method is not called in synchronized block, to make
+ * sure we are not undercounting unpersistedBytes, setting
+ * unpersistedBytes to the current number of bytes in writeBuffer.
+ *
+ * since we are calling fileChannel.force, bytes which are written to
+ * filechannel (system filecache) will be persisted to the disk. So we
+ * dont need to consider those bytes for setting value to
+ * unpersistedBytes.
+ *
+ */
+synchronized (this) {
+unpersistedBytes.set(writeBuffer.readableBytes());
Review comment:
ok, maybe add a comment that unpersistedBytes may overreport, but that's ok.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] ivankelly commented on a change in pull request #1228: Issue #570: Move logic of unpersistedbytes to bufferedchannel
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_r172447253 ## File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/LedgerManagerFactory.java ## @@ -94,7 +94,7 @@ LedgerUnderreplicationManager newLedgerUnderreplicationManager() * @param lm *Layout manager */ Review comment: What error does it give in eclipse? 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: us...@infra.apache.org With regards, Apache Git Services
[GitHub] ivankelly commented on a change in pull request #1228: Issue #570: Move logic of unpersistedbytes to bufferedchannel
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] ivankelly commented on a change in pull request #1228: Issue #570: Move logic of unpersistedbytes to bufferedchannel
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_r172317752
##
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:
boolean parameters are code smell. Code review isn't just about performance
or race conditions.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] ivankelly commented on a change in pull request #1228: Issue #570: Move logic of unpersistedbytes to bufferedchannel
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_r172313330
##
File path:
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BufferedChannel.java
##
@@ -188,4 +246,12 @@ public synchronized void clear() {
super.clear();
writeBuffer.clear();
}
-}
+
+public synchronized int getNumOfBytesInWriteBuffer() {
Review comment:
hmm, yes, it seems readableBytes() isn't thread-safe(), which is surprising.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] ivankelly commented on a change in pull request #1228: Issue #570: Move logic of unpersistedbytes to bufferedchannel
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_r172312290
##
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:
I'm strongly -1 on adding another boolean parameter to this method. You're
modifying now, so now is the time to do it cleanly.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] ivankelly commented on a change in pull request #1228: Issue #570: Move logic of unpersistedbytes to bufferedchannel
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_r172258386
##
File path:
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BufferedChannel.java
##
@@ -138,6 +182,20 @@ public long forceWrite(boolean forceMetadata) throws
IOException {
// the force write, any flush that happens after this may or may
// not be flushed
long positionForceWrite = writeBufferStartPosition.get();
+/*
+ * since forceWrite method is not called in synchronized block, to make
+ * sure we are not undercounting unpersistedBytes, setting
+ * unpersistedBytes to the current number of bytes in writeBuffer.
+ *
+ * since we are calling fileChannel.force, bytes which are written to
+ * filechannel (system filecache) will be persisted to the disk. So we
+ * dont need to consider those bytes for setting value to
+ * unpersistedBytes.
+ *
+ */
+synchronized (this) {
+unpersistedBytes.set(writeBuffer.readableBytes());
Review comment:
unpersistedBytes is called before the force, so the unpersisted bytes here
is actually just the unflushed bytes. If this is correct, then unpersistedBytes
should be updated in flushInternal. It can even be done by decrementAndGet
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] ivankelly commented on a change in pull request #1228: Issue #570: Move logic of unpersistedbytes to bufferedchannel
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_r172257226
##
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:
Looking at flushInternal(), that method should be synchronized. I would
synchronize flushInternal, and rename it to flush(), then kill these two
methods completely. Where they are being called, if the user wants to force,
they should call forceWrite directly (should probably be renamed force too).
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] ivankelly commented on a change in pull request #1228: Issue #570: Move logic of unpersistedbytes to bufferedchannel
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_r172250114
##
File path:
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BufferedChannel.java
##
@@ -188,4 +246,12 @@ public synchronized void clear() {
super.clear();
writeBuffer.clear();
}
-}
+
+public synchronized int getNumOfBytesInWriteBuffer() {
Review comment:
why synchronized?
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] ivankelly commented on a change in pull request #1228: Issue #570: Move logic of unpersistedbytes to bufferedchannel
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_r172250275
##
File path:
bookkeeper-server/src/main/java/org/apache/bookkeeper/bookie/BufferedChannel.java
##
@@ -188,4 +246,12 @@ public synchronized void clear() {
super.clear();
writeBuffer.clear();
}
-}
+
+public synchronized int getNumOfBytesInWriteBuffer() {
Review comment:
Is this used by a later patch?
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] ivankelly commented on a change in pull request #1228: Issue #570: Move logic of unpersistedbytes to bufferedchannel
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_r172255761
##
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:
why is this synchronized here? calling flush shouldForceWrite(false) is the
same as just calling flushInternal(). I would suggest breaking these into their
own methods, and getting rid of the boolean flags, so it's clear what is being
called at the callsite.
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:
us...@infra.apache.org
With regards,
Apache Git Services
[GitHub] ivankelly commented on a change in pull request #1228: Issue #570: Move logic of unpersistedbytes to bufferedchannel
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_r172252472 ## File path: bookkeeper-server/src/main/java/org/apache/bookkeeper/meta/LedgerManagerFactory.java ## @@ -94,7 +94,7 @@ LedgerUnderreplicationManager newLedgerUnderreplicationManager() * @param lm *Layout manager */ Review comment: Unrelated change. 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: us...@infra.apache.org With regards, Apache Git Services
