Add direct access option to avoid caching certain paths #18326

yuzhu · 2023-10-25T07:07:20Z

What changes are proposed in this pull request?

Datalake formats such as Iceberg requires frequent changes to certain files, it is better not to cache them at all to avoid frequent invalidations.

Why are the changes needed?

for correct functionality using iceberg

Does this PR introduce any user facing changes?

one user property that is by default empty, alluxio.user.file.direct.access

yuzhu · 2023-10-25T07:08:33Z

@dbw9580 @apc999 as I am adding more to this PR, I find that I am changing all the entry point that path-specific configuration is changing. I am not sure that this change is any less complex than path-specific config when it is fully done.

dbw9580 · 2023-10-27T07:24:25Z

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

+          .setCommonOptions(FileSystemOptionsUtils.commonDefaults(conf)
+          .toBuilder().setSyncIntervalMs(0)).build();
+      // may not be necessary
+      options = options.toBuilder().setReadType(ReadPType.NO_CACHE).build();


I think NO_CACHE means to not cache the block into the worker's storage if it's not already cached. It says nothing about skipping reading from the cache if it's already cached.

We need a new read type SKIP_CACHE to make the worker to always read from the UFS, regardless of whether the block is already cached or not.

what if we refresh/invalidate the cache in combination with NO_CACHE? i think that is effectively skip_CACHE no?

dbw9580 · 2023-11-03T04:02:08Z

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

+    if (!getConf().isSet(PropertyKey.USER_FILE_DIRECT_ACCESS)) {
+      return false;
+    }
+    List<String> pathList = getConf().getList(PropertyKey.USER_FILE_DIRECT_ACCESS);


getConf().getList(PropertyKey.USER_FILE_DIRECT_ACCESS) can be cached as this is called for every call and can be harmful to performance otherwise.

dbw9580 · 2023-11-03T04:04:15Z

core/common/src/main/java/alluxio/conf/NestedAlluxioConfiguration.java

+ * 2. inner configuration
+ */
+@ThreadSafe
+public class NestedAlluxioConfiguration implements AlluxioConfiguration {


I'd suggest overlay configuration as the name

Suggested change

public class NestedAlluxioConfiguration implements AlluxioConfiguration {

public class OverlayConfiguration implements AlluxioConfiguration {

dbw9580 · 2023-11-03T04:05:21Z

core/common/src/main/java/alluxio/conf/NestedAlluxioConfiguration.java

+  @Override
+  public Set<PropertyKey> keySet() {
+    return mInnerConf.keySet();
+  }
+
+  @Override
+  public Set<PropertyKey> userKeySet() {
+    return mOuterConf.keySet();
+  }


why keySet and userKeySet have different sources? Should it be a union of both layers?

this was set this way because I wanted Outer to completely override inner but that does not seem like the right way.

dbw9580 · 2023-11-03T04:18:18Z

core/common/src/main/java/alluxio/conf/NestedAlluxioConfiguration.java

+  @Override
+  public AlluxioProperties copyProperties() {
+    AlluxioProperties properties = mInnerConf.copyProperties();
+    for (PropertyKey key : keySet()) {


AlluxioConfiguration.keySet includes all properties defined in the PropertyKey class, even if it's not defined in this particular AlluxioProperties object. What you need is probably for (Entry<PropertyKey, Object> entry : properties.entrySet()).

dbw9580 · 2023-11-03T04:19:51Z

core/common/src/main/java/alluxio/conf/NestedAlluxioConfiguration.java

+  }
+
+  private AlluxioConfiguration conf(PropertyKey key) {
+    return mOuterConf.isSet(key) ? mOuterConf : mInnerConf;


Does this mean a default value in the outer layer gets to override a value explicitly set by the user in the inner layer?

Good catch.

take the specific example of a path specific configuration as inner and instancedConfiguration as outer. path specific will have cluster default as the default and pathspecific as the user set properties. InstancedConfiguration will have the system default as the default and user set as user set configurations.

dbw9580 · 2023-11-03T04:22:16Z

tests/src/test/java/alluxio/client/fs/DirectAccessIntegrationTest.java

+/**
+ * Integration tests for path level configurations.
+ */
+public class DirectAccessIntegrationTest {


Inherit from BaseIntegrationTest to get timeout and logging facilities.

Suggested change

public class DirectAccessIntegrationTest {

public class DirectAccessIntegrationTest extends BaseIntegrationTest {

dbw9580 · 2023-11-03T04:27:29Z

tests/src/test/java/alluxio/client/fs/DirectAccessIntegrationTest.java

+          .build();
+  private FileSystem mFileSystem;
+
+  private final String mLocalUfsPath = Files.createTempDir().getAbsolutePath();


Use AlluxioTestDirectory which automatically cleans up the directory after the test is finished. Take this as an example:

alluxio/dora/core/server/master/src/test/java/alluxio/master/file/FileSystemMasterTestBase.java

Lines 154 to 155 in 6dc8ed2

put(PropertyKey.MASTER_MOUNT_TABLE_ROOT_UFS, AlluxioTestDirectory

.createTemporaryDirectory("FileSystemMasterTest").getAbsolutePath());

dbw9580 · 2023-11-03T04:33:04Z

tests/src/test/java/alluxio/client/fs/DirectAccessIntegrationTest.java

+    try (FileInStream is = mFileSystem.openFile(uri)) {
+      IOUtils.copy(is, ByteStreams.nullOutputStream());
+    }
+    FileSystemUtils.waitForAlluxioPercentage(mFileSystem, uri, shouldCache ? 100 : 0);


In case of shouldCacheBefore == false and shouldCache == false,
waitForAlluxioPercentage returns immediately when the expected cached percentage is 0. It takes time for this percentage to increase to above 0, so if the worker incorrectly caches the data which it wasn't supposed to cache, this test will fail to detect this.

A better alternative is to look directly into the block master to see if the file has any associated blocks.

dbw9580 · 2023-11-03T04:36:56Z

core/common/src/main/java/alluxio/conf/NestedAlluxioConfiguration.java

+ * 2. inner configuration
+ */
+@ThreadSafe
+public class NestedAlluxioConfiguration implements AlluxioConfiguration {


Add unit test for this class?

yuzhu

@dbw9580 addressed comments, PTAL

yuzhu · 2023-11-03T07:02:56Z

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

+    if (!getConf().isSet(PropertyKey.USER_FILE_DIRECT_ACCESS)) {
+      return false;
+    }
+    List<String> pathList = getConf().getList(PropertyKey.USER_FILE_DIRECT_ACCESS);


yuzhu · 2023-11-03T07:03:06Z

core/common/src/main/java/alluxio/conf/NestedAlluxioConfiguration.java

+ * 2. inner configuration
+ */
+@ThreadSafe
+public class NestedAlluxioConfiguration implements AlluxioConfiguration {


yuzhu · 2023-11-03T07:21:49Z

core/common/src/main/java/alluxio/conf/NestedAlluxioConfiguration.java

+  }
+
+  private AlluxioConfiguration conf(PropertyKey key) {
+    return mOuterConf.isSet(key) ? mOuterConf : mInnerConf;


Good catch.

take the specific example of a path specific configuration as inner and instancedConfiguration as outer. path specific will have cluster default as the default and pathspecific as the user set properties. InstancedConfiguration will have the system default as the default and user set as user set configurations.

yuzhu · 2023-11-03T07:22:17Z

core/common/src/main/java/alluxio/conf/NestedAlluxioConfiguration.java

+  @Override
+  public Set<PropertyKey> keySet() {
+    return mInnerConf.keySet();
+  }
+
+  @Override
+  public Set<PropertyKey> userKeySet() {
+    return mOuterConf.keySet();
+  }


this was set this way because I wanted Outer to completely override inner but that does not seem like the right way.

dbw9580 · 2023-11-03T10:42:48Z

@yuzhu I think you forgot to push the latest changes that addresses the comments

dbw9580 · 2023-11-06T07:11:02Z

core/common/src/main/java/alluxio/conf/OverlayConfiguration.java

+ * determines the value.
+ */
+@ThreadSafe
+public class OverlayConfiguration implements AlluxioConfiguration {


Add unit test for this class?

dbw9580

mostly LGTM

yuzhu · 2023-11-06T13:28:52Z

alluxio-bot, merge this please

Xenorith · 2023-11-06T19:27:17Z

alluxio-bot, cherry-pick this to branch-2.10 please

alluxio-bot · 2023-11-06T19:27:46Z

Auto cherry-pick to branch branch-2.10 successfully opened PR: #18384

### What changes are proposed in this pull request? Datalake formats such as Iceberg requires frequent changes to certain files, it is better not to cache them at all to avoid frequent invalidations. ### Why are the changes needed? for correct functionality using iceberg ### Does this PR introduce any user facing changes? one user property that is by default empty, alluxio.user.file.direct.access pr-link: #18326 change-id: cid-25a1ee3c2876a4a4126727113342c24d40b06126

jiacheliu3 · 2023-11-08T04:55:02Z

@yuzhu Do we need this in main? Thanks!

yuzhu · 2023-11-08T06:24:16Z

Not needed in main, will require a very different implementation.

### What changes are proposed in this pull request? Datalake formats such as Iceberg requires frequent changes to certain files, it is better not to cache them at all to avoid frequent invalidations. ### Why are the changes needed? for correct functionality using iceberg ### Does this PR introduce any user facing changes? one user property that is by default empty, alluxio.user.file.direct.access pr-link: Alluxio#18326 change-id: cid-25a1ee3c2876a4a4126727113342c24d40b06126

### What changes are proposed in this pull request? Merge missing commits from master-2.x to main. The commits in 2023/07/01~2023/11/08 from main...master-2.x will be included by this PR. We do this merge to catch missing fixes from `master-2.x` and catch the train before `main` cuts a release. #17747 is not cherry picked because tencent cloud EMR doc is removed #17755 is not cherry picked because DistLoadCliRunner has been removed in 3.x #17758 is not cherry picked because MonoBlockStore has been removed in 3.x #17641 is not cherry picked because the PR has already been in main #17781 is not cherry picked because the PR has already been in main #17722 is not cherry picked because the alluxio-fuse command has been changed a lot #17489 is not cherry picked because audit log on master is no longer in 3.x #17865 is not cherry picked because replication on job service is no longer in 3.x #17858 is not cherry picked because it is already in main #18090 is not cherry picked because generate-tarball has been rewritten in 3.x #18091 is not cherry picked because the change is already in main #17474 is not cherry picked because reconfiguration feature is not defined in 3.x #17735 is not cherry picked because MonoBlockStore is no longer in 3.x #18133 is not cherry picked because the issue is about master metadata and no longer relevant in 3.x #17910 is not cherry picked because I prefer to do that manually #17983 is not cherry picked because the web UI has been reworked #17984 is not cherry picked because Mount/Unmount commands have been reworked in 3.x #18103 is not cherry picked because worker cache metrics have been reworked in 3.x #18185 is not cherry picked because the report command has been reworked in 3.x #18222 is not cherry picked because Mount/Unmount operations have been reworked in 3.x #18143 is not cherry picked because the change is already in main #18303 is not cherry picked because the change is already in main #18208 is not cherry picked because cache metrics have been reworked in 3.x #17002 is not cherry picked because the owner has been notified separately #18334 is not cherry picked because the bash scripts have been reworked in 3.x #18326 is not cherry picked because the owner has been notified separately pr-link: #18397 change-id: cid-dbf8cbb2d9e721a5a0a1e5028a3c9577438a2ac0

Cherry-pick of existing commit. orig-pr: #18326 orig-commit: 045a511 orig-commit-author: David Zhu <david@alluxio.com> pr-link: #18399 change-id: cid-25a1ee3c2876a4a4126727113342c24d40b06126

### What changes are proposed in this pull request? Datalake formats such as Iceberg requires frequent changes to certain files, it is better not to cache them at all to avoid frequent invalidations. ### Why are the changes needed? for correct functionality using iceberg ### Does this PR introduce any user facing changes? one user property that is by default empty, alluxio.user.file.direct.access pr-link: Alluxio#18326 change-id: cid-25a1ee3c2876a4a4126727113342c24d40b06126

Add direct access option to avoid caching certain paths

5a14b5a

dbw9580 reviewed Oct 27, 2023

View reviewed changes

yuzhu added 4 commits October 30, 2023 15:17

refactor to use getDirectAccessConf(path)

e76cd56

refactor to use getDirectAccessConf(path)

cc6c33b

fix test

bad3c52

fix test

4113424

yuzhu added the type-feature This issue is a feature request label Oct 31, 2023

yuzhu requested review from dbw9580 and apc999 October 31, 2023 02:51

dbw9580 suggested changes Nov 3, 2023

View reviewed changes

address comments

9e7ce5d

yuzhu commented Nov 3, 2023

View reviewed changes

fix checkstyle

f057f37

yuzhu requested a review from dbw9580 November 6, 2023 04:24

dbw9580 reviewed Nov 6, 2023

View reviewed changes

dbw9580 approved these changes Nov 6, 2023

View reviewed changes

alluxio-bot merged commit 045a511 into Alluxio:master-2.x Nov 6, 2023
19 checks passed

alluxio-bot mentioned this pull request Nov 6, 2023

Add direct access option to avoid cachi...; Port [#18326] to branch-2.10 #18384

Closed

jiacheliu3 mentioned this pull request Nov 8, 2023

Merge master-2.x commits 2023/07/01~2023/11/08 into main #18397

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add direct access option to avoid caching certain paths #18326

Add direct access option to avoid caching certain paths #18326

yuzhu commented Oct 25, 2023

yuzhu commented Oct 25, 2023

dbw9580 Oct 27, 2023

dbw9580 Oct 27, 2023

yuzhu Oct 30, 2023

dbw9580 Nov 3, 2023

yuzhu Nov 3, 2023

dbw9580 Nov 3, 2023

yuzhu Nov 3, 2023

dbw9580 Nov 3, 2023

yuzhu Nov 3, 2023

dbw9580 Nov 3, 2023

dbw9580 Nov 3, 2023

yuzhu Nov 3, 2023

dbw9580 Nov 3, 2023

dbw9580 Nov 3, 2023

dbw9580 Nov 3, 2023

dbw9580 Nov 3, 2023

dbw9580 Nov 3, 2023

yuzhu left a comment

yuzhu Nov 3, 2023

yuzhu Nov 3, 2023

yuzhu Nov 3, 2023

yuzhu Nov 3, 2023

dbw9580 commented Nov 3, 2023

dbw9580 Nov 6, 2023

dbw9580 left a comment

yuzhu commented Nov 6, 2023

Xenorith commented Nov 6, 2023

alluxio-bot commented Nov 6, 2023

jiacheliu3 commented Nov 8, 2023

yuzhu commented Nov 8, 2023

	public class NestedAlluxioConfiguration implements AlluxioConfiguration {
	public class OverlayConfiguration implements AlluxioConfiguration {

	public class DirectAccessIntegrationTest {
	public class DirectAccessIntegrationTest extends BaseIntegrationTest {

	put(PropertyKey.MASTER_MOUNT_TABLE_ROOT_UFS, AlluxioTestDirectory
	.createTemporaryDirectory("FileSystemMasterTest").getAbsolutePath());

Add direct access option to avoid caching certain paths #18326

Add direct access option to avoid caching certain paths #18326

Conversation

yuzhu commented Oct 25, 2023

What changes are proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user facing changes?

yuzhu commented Oct 25, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yuzhu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dbw9580 commented Nov 3, 2023

Choose a reason for hiding this comment

dbw9580 left a comment

Choose a reason for hiding this comment

yuzhu commented Nov 6, 2023

Xenorith commented Nov 6, 2023

alluxio-bot commented Nov 6, 2023

jiacheliu3 commented Nov 8, 2023

yuzhu commented Nov 8, 2023