[ALLUXIO-3167] Allow application level TTL #7317

Reidddddd · 2018-05-22T08:21:55Z

History:
Fix checkstyle warning
Move read ttl and ttlAction into CommonOptions
Use default free implementation temporarily

alluxio-bot · 2018-05-22T08:21:59Z

Automated checks report:

Valid pull request title: PASS
Contains link to JIRA ticket: FAIL
- Pull request body should contain a link back to the appropriate JIRA ticket (https://alluxio.atlassian.net/browse/ALLUXIO-3167)
Commits associated with Github account: PASS

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

AmplabJenkins · 2018-05-22T08:27:41Z

Merged build finished. Test FAILed.

AmplabJenkins · 2018-05-22T08:27:41Z

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

AmplabJenkins · 2018-05-22T12:09:51Z

Merged build finished. Test FAILed.

AmplabJenkins · 2018-05-22T12:09:52Z

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

Failed Tests: 13

org.alluxio:alluxio-core-server-master: 7

org.alluxio:alluxio-tests: 6

Test FAILed.

…tlAction arguments

AmplabJenkins · 2018-05-22T15:24:31Z

Merged build finished. Test PASSed.

AmplabJenkins · 2018-05-22T15:24:33Z

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

Reidddddd · 2018-05-24T02:46:55Z

ping @apc999 @aaudiber

apc999

Minor comments left. Otherwise this PR looks good. Thanks for the improvement! @Reidddddd

apc999 · 2018-05-29T18:42:24Z

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

@@ -2282,6 +2293,17 @@ public String toString() {
          .setConsistencyCheckLevel(ConsistencyCheckLevel.WARN)
          .setScope(Scope.CLIENT)
          .build();
+  public static final PropertyKey USER_FILE_WRITE_CACHE_TTL_MS =


Naming suggestion: USER_FILE_WRITE_CACHE_TTL.
We don't suffix _MS now for new configuraitons as Alluxio conf can support TTL values like "1d", "2minute", not necessary to be is unit of ms.

apc999 · 2018-05-29T22:23:45Z

core/common/src/thrift/file_system_master.thrift

-  5: optional i64 ttl
-  6: optional common.TTtlAction ttlAction
-  7: optional FileSystemMasterCommonTOptions commonOptions
+  5: optional FileSystemMasterCommonTOptions commonOptions


to keep backward compatibility, still have commonOptions with index 7 and let's not remove index 5 and 6 but simply rename them and comment the deprecation.

5: optional i64 ttlNotUsed // deprecated from 1.8 6: optional common.TTtlAction ttlActionNotUsed // deprecated from 1.8 7: optional FileSystemMasterCommonTOptions commonOptions

apc999 · 2018-05-29T22:24:22Z

core/common/src/thrift/file_system_master.thrift

-  5: optional i16 mode
-  6: optional common.TTtlAction ttlAction
-  7: optional FileSystemMasterCommonTOptions commonOptions
+  4: optional i16 mode


apc999 · 2018-05-29T22:33:41Z

core/server/master/src/main/java/alluxio/master/file/options/CreatePathOptions.java

+    getCommonOptions().setTtlAction(ttlAction);
+    return getThis();
+  }
+
  @Override
  public boolean equals(Object o) {


update this method?

Do you mean the equals() method?
Since Ttl and TtlAction are included in CommonOptions, this equals should be fine?

AmplabJenkins · 2018-05-30T04:19:46Z

Merged build finished. Test PASSed.

AmplabJenkins · 2018-05-30T04:19: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/19883/
Test PASSed.

apc999

LGTM. Thanks for this improvement!

apc999 · 2018-06-01T21:34:00Z

@aaudiber , are we good to merge this PR?

aaudiber · 2018-06-01T21:42:41Z

@calvinjia mentioned that he wanted a chance to look at this before it goes in, lets give him a chance to chime in

aaudiber

Looks good overall, left some questions and comments about property names

aaudiber · 2018-06-01T21:48:19Z

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

@@ -2282,6 +2293,17 @@ public String toString() {
          .setConsistencyCheckLevel(ConsistencyCheckLevel.WARN)
          .setScope(Scope.CLIENT)
          .build();
+  public static final PropertyKey USER_FILE_WRITE_CACHE_TTL =
+      new Builder(Name.USER_FILE_WRITE_CACHE_TTL)


Does this apply to non-cache writes using THROUGH or CACHE_THROUGH write types? If so we should remove the "cache" part

aaudiber · 2018-06-01T21:51:40Z

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

+  public static final PropertyKey USER_FILE_READ_CACHE_TTL_EXPIRED_ACTION =
+      new Builder(Name.USER_FILE_READ_CACHE_TTL_EXPIRED_ACTION)
+          .setDefaultValue("DELETE")
+          .setDescription("When file's ttl is expired, the action performs on it."


If I have a file "/file" only in the UFS, then Alluxio discovers it with TTL=1hour, TTL_ACTION=delete, does that mean that we'll delete the file from the UFS after 1 hour?

Yes, i think so.

I think write type THROUGH is the case as you described.

aaudiber · 2018-06-01T21:52:57Z

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

+          .setDescription("Time to live for files loaded from UFS by a user, no ttl by default.")
+          .build();
+  public static final PropertyKey USER_FILE_READ_CACHE_TTL_EXPIRED_ACTION =
+      new Builder(Name.USER_FILE_READ_CACHE_TTL_EXPIRED_ACTION)


we call it TTL_ACTION in other places, good to stay consistent

aaudiber · 2018-06-01T21:53:30Z

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

@@ -2253,6 +2253,17 @@ public String toString() {
          .setConsistencyCheckLevel(ConsistencyCheckLevel.WARN)
          .setScope(Scope.CLIENT)
          .build();
+  public static final PropertyKey USER_FILE_READ_CACHE_TTL =
+      new Builder(Name.USER_FILE_READ_CACHE_TTL)


I think this applies only to newly loaded files, not just and read files

Is USER_FILE_LOAD_TTL good, remove "CACHE" as well?

If we use LOAD, is it ok to rename "WRITE" to "CREATE". I think it should try to keep symmetrical, like WRITE <-> READ, CREATE <-> LOAD.

AmplabJenkins · 2018-06-05T11:06:05Z

Merged build finished. Test FAILed.

AmplabJenkins · 2018-06-05T11:06:06Z

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

Failed Tests: 1

org.alluxio:alluxio-tests: 1

alluxio.client.fs.ConcurrentDeleteIntegrationTest.sameDirConcurrentDelete

Test FAILed.

Reidddddd · 2018-06-05T11:15:36Z

How to trigger jenkins again?
{java.lang.RuntimeException: Timed out waiting for Alluxio worker to start}
It seems unrelated, but i'm not sure.

aaudiber · 2018-06-05T15:34:48Z

Jenkins, test this please

AmplabJenkins · 2018-06-05T16:05:24Z

Merged build finished. Test PASSed.

Reidddddd · 2018-06-08T03:05:57Z

Could you add documentation detailing how to manipulate TTL after this feature is added?

I think it is just like other user configurations. And i think @apc999's post [How to configure]
(https://zhuanlan.zhihu.com/p/35255287) it is pretty good and much more general and should be posted on official site.

I suggest changing the name from WRITE to CREATE

Agree, as i mentioned.

Taking it a step further, do you think there is value in differentiating between creating a new path in Alluxio vs loading one from the under storage?

Creating and loading or writing and reading often has different scenarios based on need, i tend to keep this flexibility.

AmplabJenkins · 2018-06-08T03:45:58Z

Merged build finished. Test PASSed.

AmplabJenkins · 2018-06-08T03:45:59Z

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

calvinjia · 2018-06-12T00:09:45Z

@Reidddddd Thanks for the update, I have two follow up comments

Could you change the default TTL behavior to FREE? It would be unfortunate if the user set the default TTL time but not the behavior and lost data.
The TTL changes only take into effect when we are surfacing new files from the under storage (ie. load metadata) or creating new paths (mkdir, create). To me these seem like the same concept and shouldn't need to be differentiated by two configurations. I would agree if there was a separate configuration for changing the TTL on any read request.

Reidddddd · 2018-06-12T01:03:17Z

Could you change the default TTL behavior to FREE? It would be unfortunate if the user set the default TTL time but not the behavior and lost data.

Exactly, i suffered this once in production :(. And this is what i'm going to do in next jira(at least two up coming, but are blocked by this pr), just don't like squeeze too many modifications in one pull request.

To me these seem like the same concept and shouldn't need to be differentiated by two configurations.

On concept, no argument. But for specific scenario, like writing, a user may want to keep important files without ttl, writing is not like reading whose data are persisted in UFS.

A general application always contains both read and write scenario, still don't think it a good idea to merge into one.

calvinjia · 2018-06-12T20:02:35Z

@Reidddddd Sure, could you point me to the other JIRA tickets you are planning to address?

Reidddddd · 2018-06-13T01:33:42Z

FYI https://alluxio.atlassian.net/browse/ALLUXIO-3222 @calvinjia
ping @aaudiber for approval.

aaudiber · 2018-06-15T23:06:35Z

@Reidddddd can you resolve the conflict with master? After that I think this PR is good to go

Reidddddd · 2018-06-16T01:34:05Z

sure, please wait.

AmplabJenkins · 2018-06-16T03:56:04Z

Merged build finished. Test FAILed.

AmplabJenkins · 2018-06-16T03:56:05Z

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

Failed Tests: 1

org.alluxio:alluxio-core-server-master: 1

alluxio.master.file.options.CreateDirectoryOptionsTest.equalsTest

Test FAILed.

Reidddddd · 2018-06-16T05:34:01Z

CreateDirectoryOptions{commonOptions=null, mountPoint=true, operationTimeMs=40, owner=null, group=null, mode=null, persisted=true, recursive=true, metadataLoad=true, allowExists=true} [group 1, item 1] must not be Object#equals to
CreateDirectoryOptions{commonOptions=null, mountPoint=true, operationTimeMs=40, owner=null, group=null, mode=null, persisted=true, recursive=true, metadataLoad=true, allowExists=true} [group 3, item 1]

except [group 1, item 1] and [group 3, item 1]?

Reidddddd · 2018-06-16T05:34:20Z

Jenkins, test this please

AmplabJenkins · 2018-06-16T05:55:25Z

Merged build finished. Test FAILed.

AmplabJenkins · 2018-06-16T05:55:25Z

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

Failed Tests: 1

org.alluxio:alluxio-core-server-master: 1

alluxio.master.file.options.CreateDirectoryOptionsTest.equalsTest

Test FAILed.

…ng method

AmplabJenkins · 2018-06-16T17:35:56Z

Merged build finished. Test PASSed.

AmplabJenkins · 2018-06-16T17:35:57Z

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

* [ALLUXIO-3167] Allow application level TTL * ALLUXIO-3167 Fix checkstyle warning * Move read ttl and ttlAction into CommonOptions * Use default free implementation temporarily * Fix CommonOptions's checkstyle * Missed a ) * Since CommonOptions has ttl and ttlAction, remove duplicate ttl and ttlAction arguments * Remove _MS suffix and revert thrift protocol for backward compatibility * Property name: remove 'cache', rename 'read' to 'load', use 'ttl action' * Add space between sentences * Rename '_WRITE_' to '_CREATE_' * Add UfsStatus in CreateDirectoryOptions's equals, hashCode and toString method

This reverts commit 179731a. See https://alluxio.atlassian.net/browse/ALLUXIO-3237

Reidddddd added 4 commits May 22, 2018 12:43

[ALLUXIO-3167] Allow application level TTL

04ee1b3

ALLUXIO-3167 Fix checkstyle warning

4135d8a

Move read ttl and ttlAction into CommonOptions

7f45f9f

Use default free implementation temporarily

11dd0b1

Reidddddd mentioned this pull request May 22, 2018

[ALLUXIO-3167] Allow application level TTL #7231

Closed

Reidddddd added 2 commits May 22, 2018 18:26

Fix CommonOptions's checkstyle

82202a4

Missed a )

08f1ab2

Since CommonOptions has ttl and ttlAction, remove duplicate ttl and t…

2eecafd

…tlAction arguments

apc999 reviewed May 29, 2018

View reviewed changes

Remove _MS suffix and revert thrift protocol for backward compatibility

69df08c

apc999 approved these changes May 30, 2018

View reviewed changes

aaudiber suggested changes Jun 1, 2018

View reviewed changes

Property name: remove 'cache', rename 'read' to 'load', use 'ttl action'

3cb09b4

Rename '_WRITE_' to '_CREATE_'

27ff092

calvinjia approved these changes Jun 12, 2018

View reviewed changes

aaudiber approved these changes Jun 13, 2018

View reviewed changes

Merge master branch

3415f2a

Add UfsStatus in CreateDirectoryOptions's equals, hashCode and toStri…

06bc546

…ng method

apc999 approved these changes Jun 18, 2018

View reviewed changes

apc999 merged commit 8b5afeb into Alluxio:master Jun 18, 2018

Reidddddd deleted the alluxio branch June 19, 2018 15:25

aaudiber mentioned this pull request Jun 20, 2018

[ALLUXIO-3167] Allow application level TTL (#7317) #7447

Merged

aaudiber added a commit that referenced this pull request Jun 27, 2018

Revert "[ALLUXIO-3167] Allow application level TTL (#7317) (#7447)"

aa525d9

This reverts commit 179731a. See https://alluxio.atlassian.net/browse/ALLUXIO-3237

[ALLUXIO-3167] Allow application level TTL #7317

[ALLUXIO-3167] Allow application level TTL #7317

Conversation

Reidddddd commented May 22, 2018

alluxio-bot commented May 22, 2018

AmplabJenkins commented May 22, 2018

AmplabJenkins commented May 22, 2018

AmplabJenkins commented May 22, 2018

AmplabJenkins commented May 22, 2018

Failed Tests: 13

org.alluxio:alluxio-core-server-master: 7

org.alluxio:alluxio-tests: 6

AmplabJenkins commented May 22, 2018

AmplabJenkins commented May 22, 2018

Reidddddd commented May 24, 2018

apc999 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

AmplabJenkins commented May 30, 2018

AmplabJenkins commented May 30, 2018

apc999 left a comment

Choose a reason for hiding this comment

apc999 commented Jun 1, 2018

aaudiber commented Jun 1, 2018

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

AmplabJenkins commented Jun 5, 2018

AmplabJenkins commented Jun 5, 2018

Failed Tests: 1

org.alluxio:alluxio-tests: 1

Reidddddd commented Jun 5, 2018 • edited Loading

aaudiber commented Jun 5, 2018

AmplabJenkins commented Jun 5, 2018

Reidddddd commented Jun 8, 2018 • edited Loading

AmplabJenkins commented Jun 8, 2018

AmplabJenkins commented Jun 8, 2018

calvinjia commented Jun 12, 2018

Reidddddd commented Jun 12, 2018 • edited Loading

calvinjia commented Jun 12, 2018

Reidddddd commented Jun 13, 2018

aaudiber commented Jun 15, 2018

Reidddddd commented Jun 16, 2018

AmplabJenkins commented Jun 16, 2018

AmplabJenkins commented Jun 16, 2018

Failed Tests: 1

org.alluxio:alluxio-core-server-master: 1

Reidddddd commented Jun 16, 2018

Reidddddd commented Jun 16, 2018

AmplabJenkins commented Jun 16, 2018

AmplabJenkins commented Jun 16, 2018

Failed Tests: 1

org.alluxio:alluxio-core-server-master: 1

AmplabJenkins commented Jun 16, 2018

AmplabJenkins commented Jun 16, 2018

Reidddddd commented Jun 5, 2018 •

edited

Loading

Reidddddd commented Jun 8, 2018 •

edited

Loading

Reidddddd commented Jun 12, 2018 •

edited

Loading