[ALLUXIO-262] Cache incomplete blocks during file seek #3089

@@ -78,6 +78,8 @@
  private boolean mClosed;
  /** Whether or not the current block should be cached. */
  private boolean mShouldCacheCurrentBlock;
+  /** Include incomplete blocks if Alluxio is configured to store blocks in Alluxio storage. */
+  private boolean mShouldCacheIncompleteBlock;


should this be final?

gpang · 2016-04-27T14:49:30Z

Thanks @peisun1115 for this feature! I left some comments.

calvinjia · 2016-04-28T00:12:21Z

core/client/src/main/java/alluxio/client/file/FileInStream.java

+  }
+
+  /**
+   * Seeks to a file position. Blocks are cached even if they are fully read. This is only called by


If they are not fully read?

calvinjia · 2016-04-28T00:16:14Z

@peisun1115 is the intended behavior consistent with the ticket? It seems like using this option will not cache if a block if the read is just of a small portion of the block. Also if the user seeks to a position inside a large file, all the blocks before that position will need to be cached which is not the intention right?

apc999 · 2016-04-28T06:09:45Z

core/client/src/main/java/alluxio/client/file/FileInStream.java

@@ -78,6 +78,8 @@
  private boolean mClosed;
  /** Whether or not the current block should be cached. */
  private boolean mShouldCacheCurrentBlock;
+  /** Include incomplete blocks if Alluxio is configured to store blocks in Alluxio storage. */


the javadoc is a bit unclear. do you mean:

Whether to store incomplete blocks in Alluxio ?

peisun1115 · 2016-04-28T15:53:29Z

@gpang @apc999 @calvinjia

I explained this PR and compared it with the other approach (prefetch every block) in https://docs.google.com/document/d/1mR1_NPt60smz5nWMAyeUZfXNllrKKmc39CaUORb1oPg/edit

I can change this PR to do prefetching if you guys prefer. I will address the comments once we agree on the approach.

AmplabJenkins · 2016-04-29T18:48:08Z

Merged build finished. Test PASSed.

gpang · 2016-05-06T22:24:05Z

core/client/src/main/java/alluxio/client/file/FileInStream.java

@@ -345,7 +352,7 @@ private void checkAndAdvanceBlockInStream() throws IOException {
        try {
          WorkerNetAddress address = mLocationPolicy.getWorkerForNextBlock(
              mContext.getAlluxioBlockStore().getWorkerInfoList(), getBlockSizeAllocation(mPos));
-          // Don't cache the block to somewhere that already has it.
+         // Don't cache the block to somewhere that already has it.


I think this should be changed back?

gpang · 2016-05-06T22:24:26Z

@peisun1115 Thanks! I left a few comments.

peisun1115 · 2016-05-09T05:59:25Z

@gpang I have addressed your comments.

AmplabJenkins · 2016-05-09T06:04:02Z

Merged build finished. Test FAILed.

AmplabJenkins · 2016-05-09T06:23:17Z

Merged build finished. Test PASSed.

AmplabJenkins · 2016-05-09T06:23:18Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Alluxio-Pull-Request-Builder/9652/
Test PASSed.

gpang · 2016-05-09T13:06:53Z

Thanks @peisun1115 , I just had one last question.

peisun1115 · 2016-05-09T15:40:40Z

@gpang fixed

AmplabJenkins · 2016-05-09T15:53:25Z

Merged build finished. Test PASSed.

AmplabJenkins · 2016-05-09T15:53:25Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Alluxio-Pull-Request-Builder/9657/
Test PASSed.

apc999 · 2016-05-09T23:33:08Z

core/client/src/main/java/alluxio/client/file/FileInStream.java

+      if (mPos == pos) {
+        return;
+      }
+


if we reach here, that means seeks backwards, right?
if yes, please javadoc this.

done. FYI, I am refactoring this file to make it clearer. I feel FileInStream is a little hard to follow and easy to make mistakes when I make changes to it.

apc999 · 2016-05-09T23:36:48Z

I left two post-LGTM comments, if fixed/answered, LGTM to me

peisun1115 · 2016-05-09T23:40:44Z

@apc999 Fixed. Thank you.

AmplabJenkins · 2016-05-09T23:55:20Z

Merged build finished. Test FAILed.

AmplabJenkins · 2016-05-09T23:55:21Z

Test FAILed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Alluxio-Pull-Request-Builder/9675/

Failed Tests: 1

org.alluxio:alluxio-tests: 1

alluxio.collections.IndexedSetConcurrencyTest.basicConcurrentUpdateTest

Test FAILed.

peisun1115 · 2016-05-10T00:52:15Z

Jerkins, test this please

AmplabJenkins · 2016-05-10T01:08:07Z

Merged build finished. Test PASSed.

AmplabJenkins · 2016-05-10T01:08:08Z

Test PASSed.
Refer to this link for build results (access rights to CI server needed):
https://amplab.cs.berkeley.edu/jenkins//job/Alluxio-Pull-Request-Builder/9678/
Test PASSed.

gpang · 2016-05-10T13:00:05Z

LGTM

calvinjia · 2016-05-10T16:55:33Z

LGTM

gpang reviewed Apr 27, 2016
View reviewed changes

calvinjia reviewed Apr 28, 2016
View reviewed changes

apc999 reviewed Apr 28, 2016
View reviewed changes

gpang reviewed May 6, 2016
View reviewed changes

Gene's comment

cf01821

style fix

667ae49

validPosition

be01df9

apc999 reviewed May 9, 2016
View reviewed changes

Add two more comments per Bin's request

9355e6b

peisun1115 mentioned this pull request May 10, 2016

[ALLUXIO-1942] Refactor FileInStream #3208

Merged

calvinjia merged commit 2451a6c into Alluxio:master May 10, 2016

peisun1115 deleted the CacheBlock branch May 11, 2016 01:06

[ALLUXIO-262] Cache incomplete blocks during file seek #3089

[ALLUXIO-262] Cache incomplete blocks during file seek #3089

Conversation

peisun1115 commented Apr 26, 2016

AmplabJenkins commented Apr 26, 2016

peisun1115 commented Apr 26, 2016

AmplabJenkins commented Apr 26, 2016

AmplabJenkins commented Apr 26, 2016

Failed Tests: 1

org.alluxio:alluxio-core-client: 1

AmplabJenkins commented Apr 26, 2016

AmplabJenkins commented Apr 26, 2016

AmplabJenkins commented Apr 26, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gpang commented Apr 27, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

calvinjia commented Apr 28, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

peisun1115 commented Apr 28, 2016

AmplabJenkins commented Apr 29, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gpang commented May 6, 2016

peisun1115 commented May 9, 2016

AmplabJenkins commented May 9, 2016

AmplabJenkins commented May 9, 2016

AmplabJenkins commented May 9, 2016

gpang commented May 9, 2016

peisun1115 commented May 9, 2016

AmplabJenkins commented May 9, 2016

AmplabJenkins commented May 9, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

apc999 commented May 9, 2016

peisun1115 commented May 9, 2016

AmplabJenkins commented May 9, 2016

AmplabJenkins commented May 9, 2016

Failed Tests: 1

org.alluxio:alluxio-tests: 1

peisun1115 commented May 10, 2016

AmplabJenkins commented May 10, 2016

AmplabJenkins commented May 10, 2016

gpang commented May 10, 2016

calvinjia commented May 10, 2016