[GitHub] [hbase] Apache9 commented on a change in pull request #1970: HBASE-24625 AsyncFSWAL.getLogFileSizeIfBeingWritten does not return the expected synced file length.

2020-07-06 Thread GitBox 


Apache9 commented on a change in pull request #1970:
URL: https://github.com/apache/hbase/pull/1970#discussion_r450577129



##
File path: 
hbase-asyncfs/src/main/java/org/apache/hadoop/hbase/io/asyncfs/WrapperAsyncFSOutput.java
##
@@ -91,7 +93,11 @@ private void flush0(CompletableFuture future, 
ByteArrayOutputStream buffer
   out.hflush();
 }
   }
-  future.complete(out.getPos());
+  long pos = out.getPos();
+  if(pos > this.syncedLength) {
+this.syncedLength = pos;

Review comment:
   Oh, reviewed the code again, actuall, the flush0 method can only be 
executed in a single thread so no need to use AtomicUtils.updateMax. The 
AtomicLong is in the ProtobufLogWriter, not the output stream. But the 'if(pos 
> this.syncedLength) {' is a bit confusing to developers, I prefer we just 
remove this check...





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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [hbase] Apache9 commented on a change in pull request #1970: HBASE-24625 AsyncFSWAL.getLogFileSizeIfBeingWritten does not return the expected synced file length.

2020-07-06 Thread GitBox 


Apache9 commented on a change in pull request #1970:
URL: https://github.com/apache/hbase/pull/1970#discussion_r450569339



##
File path: 
hbase-asyncfs/src/main/java/org/apache/hadoop/hbase/io/asyncfs/WrapperAsyncFSOutput.java
##
@@ -91,7 +93,11 @@ private void flush0(CompletableFuture future, 
ByteArrayOutputStream buffer
   out.hflush();
 }
   }
-  future.complete(out.getPos());
+  long pos = out.getPos();
+  if(pos > this.syncedLength) {
+this.syncedLength = pos;

Review comment:
   This one is just for test so not a big problem but aligning with other 
producation implementations is better. Can have an addendum.





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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org

[GitHub] [hbase] Apache9 commented on a change in pull request #1970: HBASE-24625 AsyncFSWAL.getLogFileSizeIfBeingWritten does not return the expected synced file length.

2020-07-02 Thread GitBox 


Apache9 commented on a change in pull request #1970:
URL: https://github.com/apache/hbase/pull/1970#discussion_r448991456



##
File path: 
hbase-server/src/main/java/org/apache/hadoop/hbase/wal/WALProvider.java
##
@@ -74,6 +74,22 @@
 
   interface WriterBase extends Closeable {
 long getLength();
+/**
+ * 

Review comment:
   Do not need to use pre here? These are just normal text.





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.

For queries about this service, please contact Infrastructure at:
us...@infra.apache.org