Skip to content

Add configs for ttl of Alluxio cache#19905

Merged
zacw7 merged 1 commit intoprestodb:masterfrom
beinan:presto_local_cache_ttl
Jun 22, 2023
Merged

Add configs for ttl of Alluxio cache#19905
zacw7 merged 1 commit intoprestodb:masterfrom
beinan:presto_local_cache_ttl

Conversation

@beinan
Copy link
Member

@beinan beinan commented Jun 16, 2023

Add support for the TTL of Alluxio SDK cache

== RELEASE NOTES ==

Hive Changes
* Add support for the TTL of Alluxio SDK cache

@beinan beinan requested a review from a team as a code owner June 16, 2023 23:38
@beinan beinan requested a review from presto-oss June 16, 2023 23:38
Copy link
Contributor

@apc999 apc999 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM modulo minor tweaks here

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better to keep them alphabetical

Suggested change
.setTtlEnabled(false)
.setTtlCheckInterval(new Duration(1, HOURS))
.setTtlCheckInterval(new Duration(1, HOURS))
.setTtlEnabled(false)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

better to keep them alphabetical

Suggested change
.put("cache.alluxio.ttl-enabled", "true")
.put("cache.alluxio.ttl-check-interval", "60s")
.put("cache.alluxio.ttl-check-interval", "60s")
.put("cache.alluxio.ttl-enabled", "true")

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
.setTtlEnabled(true)
.setTtlCheckInterval(new Duration(60, SECONDS))
.setTtlCheckInterval(new Duration(60, SECONDS))
.setTtlEnabled(true)

Copy link
Member

@zacw7 zacw7 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the prompt change!

@zacw7
Copy link
Member

zacw7 commented Jun 21, 2023

Thanks @NikhilCollooru for review.

@beinan could you please fix the checkstyle issues then we can have this change merged? Thanks!

@beinan beinan force-pushed the presto_local_cache_ttl branch from 1ad6c42 to 6edab30 Compare June 21, 2023 18:45
@zacw7 zacw7 merged commit b893d88 into prestodb:master Jun 22, 2023
@wanglinsong wanglinsong mentioned this pull request Jul 27, 2023
28 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants