Remove caching for CapacityBaseRandomPolicy #16187

dengweisysu · 2022-09-13T07:59:03Z

What changes are proposed in this pull request?

This effectively reverts #15423 (3b1bdb3)

For the problem that #15423 was trying to solve, a new policy is proposed in #16237.

Why are the changes needed?

local cache is useless because BlockLocationPolicy will be created every time, because it is not a singleton
local cache can only be useful to limit replica in each client, but useless for a cluster with many client (eg. presto workers ) because each client has different cache result.
additionally, in general, block location policies should not be concerned about block replication, as master will take care of that.

Does this PR introduce any user facing changes?

Two property keys are deprecated: alluxio.user.ufs.block.read.location.policy.cache.size and alluxio.user.ufs.block.read.location.policy.cache.expiration.time, as the cache is removed from the policy.

alluxio-bot · 2022-09-13T07:59:08Z

Automated checks report:

Commits associated with Github account: PASS
PR title follows the conventions: FAIL
- The title of the PR does not pass all the checks. Please fix the following issues:
  - First word must be capitalized

Some checks failed. Please fix the reported issues and reply 'alluxio-bot, check this please' to re-run checks.

alluxio-bot · 2022-09-13T07:59:31Z

Automated checks report:

Commits associated with Github account: PASS
PR title follows the conventions: PASS

All checks passed!

maobaolong

Left a minor question

maobaolong · 2022-09-19T01:16:08Z

core/client/fs/src/test/java/alluxio/client/block/policy/CapacityBaseRandomPolicyTest.java

      @Override
-      protected long randomInCapacity(long totalCapacity) {
+      protected long randomInCapacity(Long blockId, long totalCapacity) {


Why you change the long to Long?

dbw9580 · 2022-09-19T08:56:03Z

core/client/fs/src/main/java/alluxio/client/block/policy/CapacityBaseRandomPolicy.java

-    return null;
+    // blockId base hash value to decide which worker to cache data,
+    // so the same block will be routed to the same set of worker.
+    long sourceValue = blockId + ThreadLocalRandom.current().nextInt(mMaxReplicaSize);


I wonder why this policy wants to take block replication into consideration. I think master has a dedicated background job taking care of the replication adjustment periodically. On the other hand, the other policies like LocalFirstPolicy etc. do not care about replications, either. WDYT @jiacheliu3 @beinan?

I don't quite understand here either. @dengweisysu could you explain your routing logic here?

@dbw9580 You are right, master can take care of replications, so we can remove this random logic. But different from LocalFirstPolicy， this policy is much like DeterministicHashPolicy which use 'alluxio.user.ufs.block.read.location.policy.deterministic.hash.shards' to random target worker.

Anyway, I agree with you to remove this random logic in this policy.

jiacheliu3 · 2022-09-20T13:52:48Z

core/client/fs/src/main/java/alluxio/client/block/policy/CapacityBaseRandomPolicy.java

+    // blockId base hash value to decide which worker to cache data,
+    // so the same block will be routed to the same set of worker.


you have randomness introduced, so even if you are hashing that still doesn't give you the same set every time?

dbw9580

@dengweisysu @maobaolong as per our discussion offline, we want a separate CapacityBaseHashPolicy, and the CapacityBaseRandomPolicy will continue to be a random policy, but with the caching issue fixed.

core/client/fs/src/main/java/alluxio/client/block/policy/CapacityBaseRandomPolicy.java

dengweisysu · 2022-09-27T02:24:56Z

@dengweisysu @maobaolong as per our discussion offline, we want a separate CapacityBaseHashPolicy, and the CapacityBaseRandomPolicy will continue to be a random policy, but with the caching issue fixed.

I couldn't agree more. should I close the PR first? @dbw9580

dbw9580 · 2022-09-27T04:18:18Z

@dengweisysu @maobaolong as per our discussion offline, we want a separate CapacityBaseHashPolicy, and the CapacityBaseRandomPolicy will continue to be a random policy, but with the caching issue fixed.

I couldn't agree more. should I close the PR first? @dbw9580

Can you please make this policy a truly random one, as opposed to #16237 ? i.e. make randomInCapacity pick a random worker instead of computing the hash of the block id and pick one based on that.

dengweisysu · 2022-09-28T04:32:49Z

@dbw9580 @maobaolong is it necessary to keep the local cache logic ? I prefer to remove and rollback to the first version of CapacityBaseRandomPolicy to keep it simple. Local cache solution only solve little problem and make the the logic much more complicated.

This reverts commit 3b1bdb3

dbw9580 · 2022-09-28T07:53:52Z

@dengweisysu Yes, I just reverted 3b1bdb3

dbw9580 · 2022-09-28T08:29:50Z

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

@@ -5907,25 +5907,6 @@ public String toString() {
          .setConsistencyCheckLevel(ConsistencyCheckLevel.WARN)
          .setScope(Scope.CLIENT)
          .build();
-  public static final PropertyKey USER_UFS_BLOCK_READ_LOCATION_POLICY_CACHE_SIZE =


What's our policy for removing PropertyKeys? Should we deprecate them instead?

If alluxio client can ignore unknown config key without exception throwed, I prefer to remove it.

dbw9580

LGTM

jiacheliu3

LGTM

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

Co-authored-by: Jiacheng Liu <jiacheliu3@gmail.com>

jiacheliu3 · 2022-11-01T07:33:45Z

alluxio-bot, merge this please

### What changes are proposed in this pull request? This effectively reverts Alluxio#15423 (Alluxio@3b1bdb3) For the problem that Alluxio#15423 was trying to solve, a new policy is proposed in Alluxio#16237. ### Why are the changes needed? 1. local cache is useless because BlockLocationPolicy will be created every time, because it is not a singleton 2. local cache can only be useful to limit replica in each client, but useless for a cluster with many client (eg. presto workers ) because each client has different cache result. 3. additionally, in general, block location policies should not be concerned about block replication, as master will take care of that. ### Does this PR introduce any user facing changes? Two property keys are deprecated: `alluxio.user.ufs.block.read.location.policy.cache.size` and `alluxio.user.ufs.block.read.location.policy.cache.expiration.time`, as the cache is removed from the policy. pr-link: Alluxio#16187 change-id: cid-32c4cc50d580bbc05c31d8bdd361889e4f70f66d

### What changes are proposed in this pull request? Add a new block location policy `CapacityBaseDeterministicHashPolicy`. ### Why are the changes needed? We want a `CapacityBaseRandomPolicy` that is deterministic. See also #16187. ### Does this PR introduce any user facing changes? Yes, a new block location policy is available for config item `alluxio.user.ufs.block.read.location.policy` and `alluxio.user.block.write.location.policy.class`. pr-link: #16237 change-id: cid-47ba9b1d197b5ad546ac1a993590d49e963c3811

### What changes are proposed in this pull request? This effectively reverts Alluxio#15423 (Alluxio@3b1bdb3) For the problem that Alluxio#15423 was trying to solve, a new policy is proposed in Alluxio#16237. ### Why are the changes needed? 1. local cache is useless because BlockLocationPolicy will be created every time, because it is not a singleton 2. local cache can only be useful to limit replica in each client, but useless for a cluster with many client (eg. presto workers ) because each client has different cache result. 3. additionally, in general, block location policies should not be concerned about block replication, as master will take care of that. ### Does this PR introduce any user facing changes? Two property keys are deprecated: `alluxio.user.ufs.block.read.location.policy.cache.size` and `alluxio.user.ufs.block.read.location.policy.cache.expiration.time`, as the cache is removed from the policy. pr-link: Alluxio#16187 change-id: cid-32c4cc50d580bbc05c31d8bdd361889e4f70f66d

### What changes are proposed in this pull request? Add a new block location policy `CapacityBaseDeterministicHashPolicy`. ### Why are the changes needed? We want a `CapacityBaseRandomPolicy` that is deterministic. See also Alluxio#16187. ### Does this PR introduce any user facing changes? Yes, a new block location policy is available for config item `alluxio.user.ufs.block.read.location.policy` and `alluxio.user.block.write.location.policy.class`. pr-link: Alluxio#16237 change-id: cid-47ba9b1d197b5ad546ac1a993590d49e963c3811

…sticHashPolicy Add a new block location policy `CapacityBaseDeterministicHashPolicy`. We want a `CapacityBaseRandomPolicy` that is deterministic. See also Alluxio#16187. Yes, a new block location policy is available for config item `alluxio.user.ufs.block.read.location.policy` and `alluxio.user.block.write.location.policy.class`. pr-link: Alluxio#16237 change-id: cid-47ba9b1d197b5ad546ac1a993590d49e963c3811

dengweisysu changed the title ~~use blockId hash value to decide worker instead of local cache~~ Use blockId hash value to decide worker instead of local cache Sep 13, 2022

alluxio-bot added the API Change Changes covering public API label Sep 13, 2022

alluxio-bot assigned tcrain Sep 13, 2022

dengweisysu changed the title ~~Use blockId hash value to decide worker instead of local cache~~ Use blockId to decide worker instead of local cache in CapacityBaseRandomPolicy Sep 13, 2022

Use blockId hash value to decide worker instead of local cache

a2f321f

dengweisysu force-pushed the bugfix_capacity_policy branch from 9aefd2f to a2f321f Compare September 13, 2022 08:12

HelloHorizon requested a review from beinan September 13, 2022 22:01

maobaolong changed the title ~~Use blockId to decide worker instead of local cache in CapacityBaseRandomPolicy~~ Support to use blockId to decide worker instead of local cache in CapacityBaseRandomPolicy Sep 15, 2022

HelloHorizon requested a review from dbw9580 September 15, 2022 16:57

maobaolong added the priority-high label Sep 16, 2022

maobaolong reviewed Sep 19, 2022

View reviewed changes

dbw9580 reviewed Sep 19, 2022

View reviewed changes

jiacheliu3 reviewed Sep 20, 2022

View reviewed changes

jiacheliu3 assigned dbw9580 and unassigned tcrain Sep 23, 2022

Remove dependence of max replica size

0add647

dengweisysu force-pushed the bugfix_capacity_policy branch from bf196ce to 0add647 Compare September 23, 2022 04:37

dbw9580 mentioned this pull request Sep 26, 2022

Add CapacityBasedDeterministicHashPolicy #16237

Merged

dbw9580 reviewed Sep 26, 2022

View reviewed changes

core/client/fs/src/main/java/alluxio/client/block/policy/CapacityBaseRandomPolicy.java Outdated Show resolved Hide resolved

core/client/fs/src/main/java/alluxio/client/block/policy/CapacityBaseRandomPolicy.java Outdated Show resolved Hide resolved

Revert "Add cache for CapacityBaseRandomPolicy"

a2e8e18

This reverts commit 3b1bdb3

dbw9580 changed the title ~~Support to use blockId to decide worker instead of local cache in CapacityBaseRandomPolicy~~ Revert "Add cache for CapacityBaseRandomPolicy" Sep 28, 2022

dbw9580 reviewed Sep 28, 2022

View reviewed changes

small fix

845f35d

dbw9580 changed the title ~~Revert "Add cache for CapacityBaseRandomPolicy"~~ Remove caching for CapacityBaseRandomPolicy Oct 18, 2022

dbw9580 approved these changes Oct 20, 2022

View reviewed changes

dbw9580 added 2 commits October 26, 2022 18:29

Deprecate prop keys

9fd1196

Fix key order

0ab28c1

jiacheliu3 approved these changes Oct 31, 2022

View reviewed changes

core/common/src/main/java/alluxio/conf/PropertyKey.java Outdated Show resolved Hide resolved

core/common/src/main/java/alluxio/conf/PropertyKey.java Outdated Show resolved Hide resolved

dbw9580 mentioned this pull request Oct 31, 2022

Fix typo in the name of CapacityBaseRandomPolicy #16437

Closed

dbw9580 and others added 2 commits November 1, 2022 11:03

Update core/common/src/main/java/alluxio/conf/PropertyKey.java

9fca46e

Co-authored-by: Jiacheng Liu <jiacheliu3@gmail.com>

Update core/common/src/main/java/alluxio/conf/PropertyKey.java

f681578

Co-authored-by: Jiacheng Liu <jiacheliu3@gmail.com>

alluxio-bot merged commit 8a66a64 into Alluxio:master Nov 1, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Remove caching for CapacityBaseRandomPolicy #16187

Remove caching for CapacityBaseRandomPolicy #16187

dengweisysu commented Sep 13, 2022 •

edited by dbw9580

Loading

alluxio-bot commented Sep 13, 2022

alluxio-bot commented Sep 13, 2022

maobaolong left a comment

maobaolong Sep 19, 2022

dbw9580 Sep 19, 2022

jiacheliu3 Sep 20, 2022

dengweisysu Sep 21, 2022

jiacheliu3 Sep 20, 2022

dbw9580 left a comment •

edited

Loading

dengweisysu commented Sep 27, 2022 •

edited

Loading

dbw9580 commented Sep 27, 2022

dengweisysu commented Sep 28, 2022

dbw9580 commented Sep 28, 2022

dbw9580 Sep 28, 2022

dengweisysu Sep 28, 2022

dbw9580 left a comment

jiacheliu3 left a comment

jiacheliu3 commented Nov 1, 2022

		// blockId base hash value to decide which worker to cache data,
		// so the same block will be routed to the same set of worker.

Remove caching for CapacityBaseRandomPolicy #16187

Remove caching for CapacityBaseRandomPolicy #16187

Conversation

dengweisysu commented Sep 13, 2022 • edited by dbw9580 Loading

What changes are proposed in this pull request?

Why are the changes needed?

Does this PR introduce any user facing changes?

alluxio-bot commented Sep 13, 2022

alluxio-bot commented Sep 13, 2022

maobaolong left a comment

Choose a reason for hiding this comment

maobaolong Sep 19, 2022

Choose a reason for hiding this comment

dbw9580 Sep 19, 2022

Choose a reason for hiding this comment

jiacheliu3 Sep 20, 2022

Choose a reason for hiding this comment

dengweisysu Sep 21, 2022

Choose a reason for hiding this comment

jiacheliu3 Sep 20, 2022

Choose a reason for hiding this comment

dbw9580 left a comment • edited Loading

Choose a reason for hiding this comment

dengweisysu commented Sep 27, 2022 • edited Loading

dbw9580 commented Sep 27, 2022

dengweisysu commented Sep 28, 2022

dbw9580 commented Sep 28, 2022

dbw9580 Sep 28, 2022

Choose a reason for hiding this comment

dengweisysu Sep 28, 2022

Choose a reason for hiding this comment

dbw9580 left a comment

Choose a reason for hiding this comment

jiacheliu3 left a comment

Choose a reason for hiding this comment

jiacheliu3 commented Nov 1, 2022

dengweisysu commented Sep 13, 2022 •

edited by dbw9580

Loading

dbw9580 left a comment •

edited

Loading

dengweisysu commented Sep 27, 2022 •

edited

Loading