[ALLUXIO-2226] Add a LocalFirstPolicy that without evict action #4445

gjhkael · 2016-12-15T08:25:23Z

https://alluxio.atlassian.net/browse/ALLUXIO-2226

gjhkael · 2016-12-15T08:29:16Z

alluxio-bot, check this please

alluxio-bot · 2016-12-15T08:29:16Z

Automated checks report:

Valid pull request title: PASS
Contains link to JIRA ticket: PASS
Commits associated with Github account: PASS

All checks passed!

AmplabJenkins · 2016-12-15T08:45:48Z

Merged build finished. Test PASSed.

AmplabJenkins · 2016-12-15T08:45:48Z

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/12598/
Test PASSed.

alluxio-bot · 2016-12-15T08:45:49Z

Automated checks report:

Valid pull request title: PASS
Contains link to JIRA ticket: PASS
Commits associated with Github account: PASS

All checks passed!

aaudiber

@gjhkael Thanks for adding this new policy with good test cases! I've left a few comments on the code. Can you explain more about the purpose of the USER_FILE_WRITE_CAPACITY_RESERVED_RATIO? If possible I'd like to keep this PR simple and consider that change separately from the new policy.

aaudiber · 2016-12-15T18:34:48Z

core/client/src/test/java/alluxio/client/file/policy/LocalFirstWithoutEvictionPolicyTest.java

+    LocalFirstWithoutEvictionPolicy policy = new LocalFirstWithoutEvictionPolicy();
+    List<BlockWorkerInfo> workerInfoList = new ArrayList<>();
+    workerInfoList.add(new BlockWorkerInfo(new WorkerNetAddress().setHost("worker1")
+        .setRpcPort(PORT).setDataPort(PORT).setWebPort(PORT), Constants.GB, 0));


We don't need to set these ports, they will be zero by default which is fine for this test

aaudiber · 2016-12-15T18:38:49Z

core/client/src/test/java/alluxio/client/file/policy/LocalFirstWithoutEvictionPolicyTest.java

+    LocalFirstWithoutEvictionPolicy policy = new LocalFirstWithoutEvictionPolicy();
+    List<BlockWorkerInfo> workerInfoList = new ArrayList<>();
+    workerInfoList.add(new BlockWorkerInfo(new WorkerNetAddress().setHost("worker1")
+        .setRpcPort(PORT).setDataPort(PORT).setWebPort(PORT), Constants.GB, 0));


It looks like worker1 has 1GB available space, so it's confusing that it's not allowed to allocate a 1GB block.

Because the policy is local first, even if worker1 has the available space but local will be choice first.

sorry, meant to write this on line 73

aaudiber · 2016-12-15T18:46:34Z

docs/_data/table/en/user-configuration.yml

@@ -30,6 +30,9 @@ alluxio.user.file.worker.client.threads:
  How many threads to use for file worker clients to read from workers.
 alluxio.user.file.write.location.policy.class:
  The default location policy for choosing workers for writing a file's blocks
+alluxio.user.file.write.capacity.reserved.ratio:


Why is this property useful? We don't use anything like this for the regular LocalFirstPolicy so it seems strange to use it here. Is it possible to break adding this property into a separate PR so that it doesn't block merging the new location policy?

The property is discussed in this pull request(#4423), Because my git commit history is corrupted, So, I open the new pull request.

Thanks for the link, this makes more sense now. Instead of using a ratio, do you think it makes more sense for the configuration to specify a certain number of MB to reserve instead of a ratio, and have it default to 0?

Yes, I think it is more suitable to use a configuration to specify the number of MB to reserve.

Thanks for you review, I think this feature is demand by many people. So, I think it is of great value to users.

aaudiber · 2016-12-15T18:47:22Z

core/client/src/main/java/alluxio/client/file/policy/LocalFirstWithoutEvictionPolicy.java

+ * If No worker meets the demands, return local host.
+ */
+@ThreadSafe
+public class LocalFirstWithoutEvictionPolicy implements FileWriteLocationPolicy {


This policy could still cause eviction when all workers are out of space, so it might be better to name it LocalFirstAvoidEvictionPolicy

Seems quite reasonable.

aaudiber · 2016-12-16T02:29:31Z

core/client/src/test/java/alluxio/client/file/policy/LocalFirstWithoutEvictionPolicyTest.java

+    workerInfoList.add(new BlockWorkerInfo(new WorkerNetAddress().setHost(localhostName)
+        .setRpcPort(PORT).setDataPort(PORT).setWebPort(PORT), Constants.GB, Constants.MB));
+    Assert.assertEquals(localhostName,
+        policy.getWorkerForNextBlock(workerInfoList, Constants.GB).getHost());


if the block needs 1GB , the local worker has (1GB - 1MB) available, and the remote worker has 1GB available, should the block be written to the remote worker?

Because getAvailableBytes() is not return the actual available bytes of the worker, for the used bytes are updated only after a file commits. So, I need to reserve some space to get the available bytes. But, as you say, this policy would happened that the worker1 actually has 1GB, but the client do not get it to store 1GB data.

AmplabJenkins · 2016-12-19T04:21:43Z

Merged build finished. Test FAILed.

AmplabJenkins · 2016-12-19T04:21:43Z

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/12634/
Test FAILed.

gjhkael · 2016-12-19T06:06:27Z

@aaudiber can you review it?

AmplabJenkins · 2016-12-19T06:19:29Z

Merged build finished. Test PASSed.

AmplabJenkins · 2016-12-19T06:19:29Z

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/12637/
Test PASSed.

AmplabJenkins · 2016-12-19T06:25:04Z

Merged build finished. Test PASSed.

AmplabJenkins · 2016-12-19T06:25:04Z

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/12638/
Test PASSed.

aaudiber

Left a few more small comments, looks good overall

aaudiber · 2016-12-19T07:25:22Z

core/client/src/main/java/alluxio/client/block/BlockWorkerInfo.java

+  public long getAvailableBytes() {
+    mUserFileWriteCapacityReserved = Configuration
+        .getBytes(PropertyKey.USER_FILE_WRITE_CAPACITY_RESERVED_SIZE_BYTES);
+    return mCapacityBytes - mUsedBytes - mUserFileWriteCapacityReserved;


The space reservation logic is specific to LocalFirstAvoidEvictionPolicy, so we should put the logic there instead of modifying BlockWorkerInfo

aaudiber · 2016-12-19T07:26:15Z

core/client/src/main/java/alluxio/client/file/policy/LocalFirstAvoidEvictionPolicy.java

+/**
+ * A policy that returns local host first, and if the local worker doesn't have enough availability,
+ * it randomly picks a worker from the active workers list for each block write.
+ * If No worker meets the demands, return local host.


Can you also specify the behavior of USER_FILE_WRITE_CAPACITY_RESERVED_SIZE_BYTES?

aaudiber · 2016-12-19T07:28:22Z

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

@@ -252,6 +252,8 @@
      Name.USER_FILE_WORKER_CLIENT_POOL_GC_THRESHOLD_MS, 300 * Constants.SECOND_MS),
  USER_FILE_WRITE_LOCATION_POLICY(Name.USER_FILE_WRITE_LOCATION_POLICY,
      "alluxio.client.file.policy.LocalFirstPolicy"),
+  USER_FILE_WRITE_CAPACITY_RESERVED_SIZE_BYTES(


This property only modifies the new policy, so what do you think of naming it
USER_FILE_WRITE_AVOID_EVICTION_POLICY_RESERVED_BYTES?

aaudiber · 2016-12-19T07:28:46Z

docs/_data/table/en/user-configuration.yml

@@ -30,6 +30,9 @@ alluxio.user.file.worker.client.threads:
  How many threads to use for file worker clients to read from workers.
 alluxio.user.file.write.location.policy.class:
  The default location policy for choosing workers for writing a file's blocks
+alluxio.user.file.write.capacity.reserved.size.bytes:
+  The portion of space reserced in worker when user use the LocalFirstAvoidEvictionPolicy class


reserced -> reserved

Sorry about this small mistake.

AmplabJenkins · 2016-12-19T08:19:50Z

Merged build finished. Test PASSed.

AmplabJenkins · 2016-12-19T08:19:50Z

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/12641/
Test PASSed.

… configuration key

gjhkael · 2016-12-19T08:56:28Z

alluxio-bot, check this please

AmplabJenkins · 2016-12-19T09:14:47Z

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/12642/
Test PASSed.

AmplabJenkins · 2016-12-19T09:29:53Z

Merged build finished. Test PASSed.

AmplabJenkins · 2016-12-19T09:29:54Z

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/12643/
Test PASSed.

aaudiber

LGTM, thanks for adding this much-requested policy!

yupeng9

Thanks for adding this useful policy. Left some minor comments

yupeng9 · 2016-12-19T18:22:43Z

core/client/src/main/java/alluxio/client/file/policy/LocalFirstAvoidEvictionPolicy.java

+ * to store the block, for the values mCapacityBytes minus mUsedBytes is not the available bytes.
+ */
+@ThreadSafe
+public class LocalFirstAvoidEvictionPolicy implements FileWriteLocationPolicy {


Thanks for your reminder.

yupeng9 · 2016-12-19T18:23:06Z

core/client/src/main/java/alluxio/client/file/policy/LocalFirstAvoidEvictionPolicy.java

+ * A policy that returns local host first, and if the local worker doesn't have enough availability,
+ * it randomly picks a worker from the active workers list for each block write.
+ * If No worker meets the demands, return local host.
+ * USER_FILE_WRITE_AVOID_EVICTION_POLICY_RESERVED_BYTES is use to reserved some space of the worker


is used to reserve

AmplabJenkins · 2016-12-20T02:14:04Z

Merged build finished. Test PASSed.

AmplabJenkins · 2016-12-20T02:14:05Z

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/12674/
Test PASSed.

gpang

@gjhkael Thanks for this feature! I left a few comments.

gpang · 2016-12-21T16:17:07Z

core/client/src/main/java/alluxio/client/file/policy/LocalFirstAvoidEvictionPolicy.java

+        return workerInfo.getNetAddress();
+      }
+    }
+    return localWorkerNetAddress;


What if there was no worker on the local host? That means localWorkerNetAddress would have been null. Should null be returned, or should a random worker still be picked?

@gpang After long deliberation, if no worker on the local host and have on worker have available capacity to store the block, it should pick a random worker. When no worker in this cluster, it should return null. Thanks for you remind, it make sense.

gpang · 2016-12-21T16:17:37Z

core/client/src/main/java/alluxio/client/file/policy/LocalFirstAvoidEvictionPolicy.java

+
+  /**
+   * @param workerInfo BlockWorkerInfo of the worker
+   * @return the available bytes of the worker


Please comment how USER_FILE_WRITE_AVOID_EVICTION_POLICY_RESERVED_BYTES is used to compute the available bytes of the worker

My original idea is use the variable mCapacityBytes minus mUsedBytes of class BlockWorkerInfo to get the valid Bytes of the worker. But yupeng9 tell me that this way does not actually get the available Bytes for the information of class BlockWorkerInfo is update only after a file is completely write. So, he suggest me to reserve some space to store the block.

I will comment above describe to the method. Thanks for you advise.

AmplabJenkins · 2016-12-22T03:15:21Z

Merged build finished. Test PASSed.

AmplabJenkins · 2016-12-22T03:15:22Z

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/12744/
Test PASSed.

gjhkael · 2016-12-29T03:30:11Z

How to resolve conflicts?

AmplabJenkins · 2016-12-29T03:47:44Z

Build finished. Test FAILed.

AmplabJenkins · 2016-12-29T03:47:45Z

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/12829/

Failed Tests: 1

org.alluxio:alluxio-core-client: 1

alluxio.client.file.policy.LocalFirstAvoidEvictionPolicyTest.getLocalWhenNoneHasSpace

Test FAILed.

AmplabJenkins · 2016-12-29T07:44:00Z

Build finished. Test PASSed.

AmplabJenkins · 2016-12-29T07:44:00Z

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/12830/
Test PASSed.

calvinjia · 2017-01-09T22:18:58Z

@gjhkael You can run git merge master which will automatically merge the non-conflicting changes. You will then need to manually select which changes you want to keep (or if you want to keep parts of both) for the conflicting portions. Here is the github documentation: https://help.github.com/articles/resolving-a-merge-conflict-using-the-command-line/

AmplabJenkins · 2017-01-10T02:16:54Z

Build finished. Test PASSed.

AmplabJenkins · 2017-01-10T02:16:54Z

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/13036/
Test PASSed.

AmplabJenkins · 2017-01-10T02:20:04Z

Merged build finished. Test PASSed.

AmplabJenkins · 2017-01-10T02:20:04Z

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/13037/
Test PASSed.

gjhkael · 2017-01-10T06:33:18Z

@calvinjia thanks for you help.

gpang

@gjhkael Thanks for this feature!

LGTM

aaudiber

LGTM

gjhkael added 5 commits December 16, 2016 00:18

Add a LocalFirstPolicy that without evict action

64cd763

Add a LocalFirstPolicy that without evict action

3bdc29e

add user file write capacity reserved ratio

8be64bc

add describe of alluxio.user.file.write.capacity.reserved.ratio

6bf0036

add LocalFirstWithoutEvictionPolicy test

536e530

aaudiber suggested changes Dec 15, 2016

View reviewed changes

aaudiber reviewed Dec 16, 2016

View reviewed changes

Fixed few minor review comments

f8516e6

gjhkael added 2 commits December 19, 2016 13:59

Fixed Checkstyle errors

9a16b08

Update user-configuration

065e2b1

aaudiber suggested changes Dec 19, 2016

View reviewed changes

Fixed a word

b882567

gjhkael added 3 commits December 19, 2016 16:48

Remove the method getAvailableCapacity in BlockWorkerInfo

d4dc9e1

update the user write file local first avoid eviction policy reserved…

3e50fdb

… configuration key

add the method getAvailableBytes

6bddccf

aaudiber approved these changes Dec 19, 2016

View reviewed changes

yupeng9 reviewed Dec 19, 2016

View reviewed changes

Add modifier final to public class

c80466e

gpang suggested changes Dec 21, 2016

View reviewed changes

Add describe info of getAvailableBytes

716452f

If no worker on the local should return a random worker

a263a4e

Fix test bug

6699389

gjhkael added 2 commits January 10, 2017 09:51

Fixed some conflicts

26ef84c

Fixed some conflicts

d38b7e6

gpang approved these changes Jan 25, 2017

View reviewed changes

aaudiber approved these changes Jan 25, 2017

View reviewed changes

aaudiber merged commit 2046ad8 into Alluxio:master Jan 25, 2017

[ALLUXIO-2226] Add a LocalFirstPolicy that without evict action #4445

[ALLUXIO-2226] Add a LocalFirstPolicy that without evict action #4445

Conversation

gjhkael commented Dec 15, 2016

gjhkael commented Dec 15, 2016

alluxio-bot commented Dec 15, 2016

AmplabJenkins commented Dec 15, 2016

AmplabJenkins commented Dec 15, 2016

alluxio-bot commented Dec 15, 2016

aaudiber 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

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

gjhkael Dec 16, 2016 • edited Loading

Choose a reason for hiding this comment

AmplabJenkins commented Dec 19, 2016

AmplabJenkins commented Dec 19, 2016

gjhkael commented Dec 19, 2016

AmplabJenkins commented Dec 19, 2016

AmplabJenkins commented Dec 19, 2016

AmplabJenkins commented Dec 19, 2016

AmplabJenkins commented Dec 19, 2016

aaudiber 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

Choose a reason for hiding this comment

AmplabJenkins commented Dec 19, 2016

AmplabJenkins commented Dec 19, 2016

gjhkael commented Dec 19, 2016

AmplabJenkins commented Dec 19, 2016

AmplabJenkins commented Dec 19, 2016

AmplabJenkins commented Dec 19, 2016

aaudiber left a comment

Choose a reason for hiding this comment

yupeng9 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

AmplabJenkins commented Dec 20, 2016

AmplabJenkins commented Dec 20, 2016

gpang 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

gjhkael Dec 22, 2016 • edited Loading

Choose a reason for hiding this comment

AmplabJenkins commented Dec 22, 2016

AmplabJenkins commented Dec 22, 2016

gjhkael commented Dec 29, 2016

AmplabJenkins commented Dec 29, 2016

AmplabJenkins commented Dec 29, 2016

Failed Tests: 1

org.alluxio:alluxio-core-client: 1

AmplabJenkins commented Dec 29, 2016

AmplabJenkins commented Dec 29, 2016

calvinjia commented Jan 9, 2017

AmplabJenkins commented Jan 10, 2017

AmplabJenkins commented Jan 10, 2017

AmplabJenkins commented Jan 10, 2017

AmplabJenkins commented Jan 10, 2017

gjhkael commented Jan 10, 2017

gpang left a comment

Choose a reason for hiding this comment

gjhkael Dec 16, 2016 •

edited

Loading

gjhkael Dec 22, 2016 •

edited

Loading