Implement unbuffer interface for HdfsFileInputStream #16017

qian0817 · 2022-08-05T02:10:59Z

What changes are proposed in this pull request?

Implement unbuffer interface for HdfsFileInputStream. Fix #16016.

Why are the changes needed?

If the unbuffer method is not implemented, then impala will not be able to use the file handle cache.

Does this PR introduce any user facing changes?

Implement CanUnbuffer and StreamCapabilities for HdfsFileInputStream.

Jackson-Wang-7

Sorry, I'm not familiar with unbuffer. Firstly I want to know how and when does the client call the unbuffer method. Secondly I want to know how it affect the performance and what the optimized benefits are.
And I need to make sure it doesn't affect other scenes, please give me some more information and test examples.
Thanks!

core/client/hdfs3/pom.xml

shaded/client-hadoop3/pom.xml

core/client/fs/src/main/java/alluxio/client/file/AlluxioFileInStream.java

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

core/client/hdfs3/src/main/java/alluxio/hadoop/HdfsFileInputStream.java

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

core/client/hdfs/src/main/java/alluxio/hadoop/HdfsFileInputStream.java

qian0817 · 2022-12-05T12:47:14Z

@Jackson-Wang-7 Thanks for you review. I rebase the master branch and commit our latest changes.

Jackson-Wang-7 · 2022-12-14T07:21:03Z

Mostly LGTM, and I suggest you’d better find one more person to take a look.

jiacheliu3 · 2022-12-15T15:28:36Z

/ping @jja725 PTAL thanks!

jja725

Mostly LGTM. I've discussed this with @beinan and we're good with the new shaded method. Bring in @LuQQiu to see whether they are OK with maintaining two different ways of shaded.

jja725 · 2022-12-15T19:16:58Z

core/client/hdfs3/pom.xml

+    <parent>
+        <groupId>org.alluxio</groupId>
+        <artifactId>alluxio-core-client</artifactId>
+        <version>2.10.0-SNAPSHOT</version>


@Xenorith do we need to update the script for this new pom file?

no need, the script looks for the version string in all pom.xml files:

function update_poms() { find . -name pom.xml | xargs -t -n 1 perl -pi -e "s/${1}/${2}/g" }

jja725 · 2022-12-15T19:18:01Z

core/client/hdfs3/pom.xml

+    <artifactId>alluxio-core-client-hdfs3</artifactId>
+    <packaging>jar</packaging>
+    <name>Alluxio Core - Client - HDFS3</name>
+    <description>HDFS Client of Alluxio Core For HDFS 3</description>


Please help us update the doc regarding the hdfs3 client

Good point! Let's align first internally then comment on this PR on the recommended doc change.

the new client/hdfs3 and shaded/client-hadoop3 modules are currently a copy of the existing client/hdfs and shaded/client modules the addition of this will allow for changes that are available only in hadoop3, such as Alluxio#16017 (comment) both client jars will be built by default, but the symlink at client/alluxio-VERSION-client.jar will point to the hadoop-2 one to maintain backcompat. if the hadoop-3 profile is activated by adding `-Phadoop-3`, then the symlink will be overridden to point to the new hadoop3 shaded client jar note that in the current state of this PR, the tarball generation will result in pointing to the hadoop3 shaded client jar because the hadoop-3 profile is activated by default

the new client/hdfs3 and shaded/client-hadoop3 modules are currently a copy of the existing client/hdfs and shaded/client modules the addition of this will allow for changes that are available only in hadoop3, such as #16017 (comment) both client jars will be built by default, but the symlink at client/alluxio-VERSION-client.jar will point to the hadoop-2 one to maintain backcompat. if the hadoop-3 profile is activated by adding `-Phadoop-3`, then the symlink will be overridden to point to the new hadoop3 shaded client jar pr-link: #16699 change-id: cid-a6ffd09414e8259078fd5a8f68c3e287d85feec5

Xenorith · 2023-01-18T22:26:38Z

#16699 has merged to unblock the maven/pom parts of this change

jiacheliu3 · 2023-01-19T12:09:55Z

@qian0817 Do you mind rebasing on latest master? Let's only keep the unbuffer change in this PR, while we rely on the #16699 to build the clients we want. Feel free to let me know if there's any question, thanks!

the new client/hdfs3 and shaded/client-hadoop3 modules are currently a copy of the existing client/hdfs and shaded/client modules the addition of this will allow for changes that are available only in hadoop3, such as Alluxio#16017 (comment) both client jars will be built by default, but the symlink at client/alluxio-VERSION-client.jar will point to the hadoop-2 one to maintain backcompat. if the hadoop-3 profile is activated by adding `-Phadoop-3`, then the symlink will be overridden to point to the new hadoop3 shaded client jar pr-link: Alluxio#16699 change-id: cid-a6ffd09414e8259078fd5a8f68c3e287d85feec5

qian0817 · 2023-01-29T05:37:08Z

@jiacheliu3 @jja725 @Jackson-Wang-7 @dbw9580 Please revirwe this PR again, thanks!

Jackson-Wang-7 · 2023-02-15T03:28:56Z

Is there any new change from my LGTM last time except the pom change?

qian0817 · 2023-02-15T04:34:50Z

Is there any new change from my LGTM last time except the pom change?
No change.

Jackson-Wang-7

LGTM

jja725 · 2023-02-15T18:41:23Z

Just curious is there any other compute framework use this function except impala?

qian0817 · 2023-02-16T01:55:51Z

From HADOOP-14747 we can know that HBase also use the unbffer function

jiacheliu3

LGTM, thanks for the work!

jiacheliu3 · 2023-02-17T00:21:49Z

core/client/fs/src/test/java/alluxio/client/file/cache/LocalCacheFileInStreamTest.java

+    byte[] cacheMiss = new byte[partialReadSize];
+    stream.unbuffer();
+    stream.seek(offset);
+    stream.unbuffer();
+    Assert.assertEquals(partialReadSize, stream.read(cacheMiss));
+    stream.unbuffer();
+    Assert.assertArrayEquals(
+        Arrays.copyOfRange(testData, offset, offset + partialReadSize), cacheMiss);
+    Assert.assertEquals(0, manager.mPagesServed);
+    Assert.assertEquals(1, manager.mPagesCached);
+
+    byte[] cacheHit = new byte[partialReadSize];
+    stream.unbuffer();
+    stream.seek(offset);
+    stream.unbuffer();
+    Assert.assertEquals(partialReadSize, stream.read(cacheHit));
+    stream.unbuffer();
+    Assert.assertArrayEquals(
+        Arrays.copyOfRange(testData, offset, offset + partialReadSize), cacheHit);
+    Assert.assertEquals(1, manager.mPagesServed);


could you add some more comments in the test explaining the behavior?

The logic here is essentially the same as readPartialPage, which is to test that unbffer method does not affect the read behavior.

dbw9580

LGTM

jja725

LGTM

jiacheliu3 · 2023-02-23T01:00:25Z

alluxio-bot, merge this please

### What changes are proposed in this pull request? Implement unbuffer interface for HdfsFileInputStream. Fix Alluxio#16016. ### Why are the changes needed? If the unbuffer method is not implemented, then impala will not be able to use the file handle cache. ### Does this PR introduce any user facing changes? Implement CanUnbuffer and StreamCapabilities for HdfsFileInputStream. pr-link: Alluxio#16017 change-id: cid-b50163c7b4f199b8a61d5818a0e4739039f2745c

…ed yet: Add client-hadoop3 module the new client/hdfs3 and shaded/client-hadoop3 modules are currently a copy of the existing client/hdfs and shaded/client modules the addition of this will allow for changes that are available only in hadoop3, such as Alluxio#16017 (comment) both client jars will be built by default, but the symlink at client/alluxio-VERSION-client.jar will point to the hadoop-2 one to maintain backcompat. if the hadoop-3 profile is activated by adding `-Phadoop-3`, then the symlink will be overridden to point to the new hadoop3 shaded client jar pr-link: Alluxio#16699 change-id: cid-a6ffd09414e8259078fd5a8f68c3e287d85feec5

…dfsFileInputStream Implement unbuffer interface for HdfsFileInputStream. Fix Alluxio#16016. If the unbuffer method is not implemented, then impala will not be able to use the file handle cache. Implement CanUnbuffer and StreamCapabilities for HdfsFileInputStream. pr-link: Alluxio#16017 change-id: cid-b50163c7b4f199b8a61d5818a0e4739039f2745c

alluxio-bot added API Change Changes covering public API POM Change labels Aug 5, 2022

alluxio-bot assigned tcrain Aug 5, 2022

maobaolong mentioned this pull request Oct 10, 2022

Support freeing block for specified worker #15956

Closed

jiacheliu3 requested review from dbw9580 and Jackson-Wang-7 October 12, 2022 10:11

Jackson-Wang-7 reviewed Dec 5, 2022

View reviewed changes

qian0817 force-pushed the unbuffer branch from e8dd693 to 9fd8fa5 Compare December 5, 2022 11:38

jiacheliu3 requested a review from Jackson-Wang-7 December 8, 2022 13:27

jiacheliu3 added the priority-high label Dec 9, 2022

jja725 self-requested a review December 9, 2022 07:58

jja725 reviewed Dec 15, 2022

View reviewed changes

Xenorith mentioned this pull request Jan 3, 2023

Add client-hadoop3 module #16699

Merged

feat: implement unbuffer method.

e7a7d3d

qian0817 force-pushed the unbuffer branch from 9d39ff2 to e7a7d3d Compare January 29, 2023 03:42

alluxio-bot removed the POM Change label Jan 29, 2023

Jackson-Wang-7 approved these changes Feb 15, 2023

View reviewed changes

jiacheliu3 approved these changes Feb 17, 2023

View reviewed changes

dbw9580 approved these changes Feb 17, 2023

View reviewed changes

jja725 self-requested a review February 22, 2023 19:54

jja725 approved these changes Feb 22, 2023

View reviewed changes

alluxio-bot merged commit 903269f into Alluxio:master Feb 23, 2023

qian0817 deleted the unbuffer branch February 23, 2023 02:02

Xenorith added the area-client Alluxio client label Feb 28, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement unbuffer interface for HdfsFileInputStream #16017

Implement unbuffer interface for HdfsFileInputStream #16017

qian0817 commented Aug 5, 2022 •

edited

Loading

Jackson-Wang-7 left a comment

qian0817 commented Dec 5, 2022

Jackson-Wang-7 commented Dec 14, 2022

jiacheliu3 commented Dec 15, 2022

jja725 left a comment

jja725 Dec 15, 2022

Xenorith Dec 16, 2022

jja725 Dec 15, 2022

jiacheliu3 Dec 16, 2022

Xenorith commented Jan 18, 2023

jiacheliu3 commented Jan 19, 2023

qian0817 commented Jan 29, 2023

Jackson-Wang-7 commented Feb 15, 2023 •

edited

Loading

qian0817 commented Feb 15, 2023

Jackson-Wang-7 left a comment

jja725 commented Feb 15, 2023

qian0817 commented Feb 16, 2023

jiacheliu3 left a comment

jiacheliu3 Feb 17, 2023

qian0817 Feb 22, 2023

dbw9580 left a comment

jja725 left a comment

jiacheliu3 commented Feb 23, 2023

Implement unbuffer interface for HdfsFileInputStream #16017

Implement unbuffer interface for HdfsFileInputStream #16017

Conversation

qian0817 commented Aug 5, 2022 • edited Loading

What changes are proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user facing changes?

Jackson-Wang-7 left a comment

Choose a reason for hiding this comment

qian0817 commented Dec 5, 2022

Jackson-Wang-7 commented Dec 14, 2022

jiacheliu3 commented Dec 15, 2022

jja725 left a comment

Choose a reason for hiding this comment

jja725 Dec 15, 2022

Choose a reason for hiding this comment

Xenorith Dec 16, 2022

Choose a reason for hiding this comment

jja725 Dec 15, 2022

Choose a reason for hiding this comment

jiacheliu3 Dec 16, 2022

Choose a reason for hiding this comment

Xenorith commented Jan 18, 2023

jiacheliu3 commented Jan 19, 2023

qian0817 commented Jan 29, 2023

Jackson-Wang-7 commented Feb 15, 2023 • edited Loading

qian0817 commented Feb 15, 2023

Jackson-Wang-7 left a comment

Choose a reason for hiding this comment

jja725 commented Feb 15, 2023

qian0817 commented Feb 16, 2023

jiacheliu3 left a comment

Choose a reason for hiding this comment

jiacheliu3 Feb 17, 2023

Choose a reason for hiding this comment

qian0817 Feb 22, 2023

Choose a reason for hiding this comment

dbw9580 left a comment

Choose a reason for hiding this comment

jja725 left a comment

Choose a reason for hiding this comment

jiacheliu3 commented Feb 23, 2023

qian0817 commented Aug 5, 2022 •

edited

Loading

Jackson-Wang-7 commented Feb 15, 2023 •

edited

Loading