Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add support for Filelist caching for symlink tables #19145

Merged
merged 1 commit into from
Mar 12, 2024
Merged

Add support for Filelist caching for symlink tables #19145

merged 1 commit into from
Mar 12, 2024

Conversation

agrawalreetika
Copy link
Member

Test plan - (Please fill in how you tested your changes)

Using the hive catalog access the symlink tables by adding following properties:

hive.file-status-cache-expire-time=24h
hive.file-status-cache-size=100000000
hive.file-status-cache-tables=*

Metrics Queries -

presto> SELECT * FROM jmx.current."com.facebook.presto.hive:name=<catalog>,type=cachingdirectorylister";
presto> SELECT "listlocatedstatuscalls.totalcount", "listobjectscalls.totalcount", "liststatuscalls.totalcount" from jmx.current."com.facebook.presto.hive.s3:name=<catalog>,type=prestos3filesystem";

== RELEASE NOTES ==

Hive Changes
* Add support for Filelist caching for symlink tables

@agrawalreetika agrawalreetika requested a review from a team as a code owner March 7, 2023 07:19
@ethanyzhang ethanyzhang requested a review from imjalpreet March 21, 2023 04:44
imjalpreet
imjalpreet previously approved these changes Mar 21, 2023
View reviewed changes
Copy link
Member

@imjalpreet imjalpreet left a comment

Choose a reason for hiding this comment

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

LGTM 👍🏼

@agrawalreetika agrawalreetika force-pushed the symlinktable-listcache branch 5 times, most recently from b1f3d6b to f5bb0b9 Compare March 31, 2023 11:04
zhenxiao
zhenxiao reviewed Apr 3, 2023
View reviewed changes
Copy link
Collaborator

@zhenxiao zhenxiao left a comment

Choose a reason for hiding this comment

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

looks good, @agrawalreetika
is it possible to add a test case?

@agrawalreetika agrawalreetika force-pushed the symlinktable-listcache branch from f5bb0b9 to 805cb6b Compare June 19, 2023 18:55
@agrawalreetika
Copy link
Member Author

I'm sorry, I missed this earlier.
@zhenxiao, I have updated the commit with tests. Please review.

imjalpreet
imjalpreet reviewed Jun 19, 2023
View reviewed changes
Copy link
Member

@imjalpreet imjalpreet left a comment

Choose a reason for hiding this comment

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

Just a few minor comments

presto-hive/src/test/resources/hive_symlink_table/_symlink_format_manifest/manifest Outdated Show resolved Hide resolved
presto-hive/src/test/java/com/facebook/presto/hive/TestSymlinkTableListCaching.java Outdated Show resolved Hide resolved
@agrawalreetika agrawalreetika force-pushed the symlinktable-listcache branch from 805cb6b to 82da34f Compare June 20, 2023 05:54
@agrawalreetika agrawalreetika force-pushed the symlinktable-listcache branch from 82da34f to f10b2db Compare June 20, 2023 08:52
@agrawalreetika
Copy link
Member Author

Hi @zhenxiao, Could you please take a pass on this?

zhenxiao
zhenxiao previously approved these changes Jul 12, 2023
View reviewed changes
Copy link
Collaborator

@zhenxiao zhenxiao left a comment

Choose a reason for hiding this comment

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

looks good

@agrawalreetika agrawalreetika self-assigned this Jul 12, 2023
@agrawalreetika agrawalreetika force-pushed the symlinktable-listcache branch from f10b2db to 78faeca Compare July 12, 2023 08:49
@agrawalreetika
Copy link
Member Author

@zhenxiao Looks like after rebasing, the Jmx values in the test are not coming up after caching. But earlier these JMX metrics were populating fine. And this test runs fine from my Local IDE.

@zhenxiao
Copy link
Collaborator

yep, let's try fix the testcase. Then we could merge :)

@agrawalreetika agrawalreetika dismissed stale reviews from zhenxiao and imjalpreet via 78faeca February 22, 2024 14:49
@agrawalreetika agrawalreetika force-pushed the symlinktable-listcache branch 5 times, most recently from aebbb26 to d8157eb Compare March 12, 2024 04:52
@agrawalreetika agrawalreetika force-pushed the symlinktable-listcache branch from d8157eb to 11cf2a6 Compare March 12, 2024 05:12
zhenxiao
zhenxiao approved these changes Mar 12, 2024
View reviewed changes
Copy link
Collaborator

@zhenxiao zhenxiao left a comment

Choose a reason for hiding this comment

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

looks good

@zhenxiao zhenxiao merged commit 88cda16 into prestodb:master Mar 12, 2024
56 checks passed
@agrawalreetika agrawalreetika deleted the symlinktable-listcache branch March 12, 2024 15:54
@elharo elharo mentioned this pull request Apr 8, 2024
Closed
@wanglinsong wanglinsong mentioned this pull request May 1, 2024
Merged
48 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@imjalpreet imjalpreet imjalpreet left review comments

@nmahadevuni nmahadevuni nmahadevuni left review comments

@zhenxiao zhenxiao zhenxiao approved these changes

@presto-oss presto-oss Awaiting requested review from presto-oss presto-oss is a code owner automatically assigned from prestodb/committers

Assignees

@agrawalreetika agrawalreetika

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@agrawalreetika @zhenxiao @imjalpreet @nmahadevuni