Verify the file presense for cached directory lister and retry #20414

i-93 · 2024-01-18T17:50:36Z

Description

These changes address the problem when the new files in manifest are not visible by directoryLister immediately because of the caching delay (see #20344).

The buildManifestFileIterator() method now verifies if the referenced file does not appear in the listing. If that is the case it tries to invalidate the cache and reload the listing.

Release notes

( ) This is not user-visible or is docs only, and no release notes are required.
(*) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text:

cla-bot · 2024-01-18T17:50:39Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

cla-bot · 2024-01-19T11:39:13Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

cla-bot · 2024-01-19T12:28:50Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

cla-bot · 2024-01-19T12:52:38Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

cla-bot · 2024-01-19T13:07:09Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

mosabua · 2024-01-22T18:11:48Z

@i-93 could you submit a signed CLA please.

mosabua · 2024-01-22T23:06:46Z

@raunaqmorarka @electrum .. any idea who could help with review here?

cla-bot · 2024-01-23T20:11:48Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

findinpath · 2024-01-24T05:18:59Z

plugin/trino-hive/src/main/java/io/trino/plugin/hive/BackgroundHiveSplitLoader.java

-        Stream<TrinoFileStatus> fileStream = paths.stream()
+
+        // If file statuses came from cache verify that all are present
+        if (isCached) {


It doesn't feel right to me to ask each time whether the location is cached.
You are adding handling for a corner case in the happy flow this way.

Maybe it would be better to add a procedure to clear the directory listing caching for a specified location.

The code under this 'if' verifies if all the listed files are present in directory listing. We know that the discrepancy could be caused by the stale cache and in this case there is a way to handle it (invalidate the cache and retry). There is no sense to do it if location is not cached, invalidation is NoOp and retry would provide the same results.

I did add invalidate(Location) call to directory lister, so the conditional code would work in any case. It is just a performance optimization: avoiding verification and retrying if those are not going to change anything anyway.

i-93 · 2024-01-25T20:30:59Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

alexjo2144 · 2024-01-25T20:58:07Z

plugin/trino-hive/src/main/java/io/trino/plugin/hive/BackgroundHiveSplitLoader.java

+
+        // If file statuses came from cache verify that all are present
+        if (isCached) {
+            boolean missing = paths.stream()


Rather than fully reloading the whole cache it'd be nice if we could just check any missing paths directly.

Yes, that would be great. Unfortunately directory lister can't list the files individually, it does it by folders and caches the same way. We are invalidating the cache for a parent folder (not the whole cache!) causing the reloading of it's content.

i-93 · 2024-01-26T20:08:29Z

@i-93 could you submit a signed CLA please.

@mosabua I submit it 3 weeks ago. No response...

cla-bot · 2024-01-27T12:20:40Z

Thank you for your pull request and welcome to the Trino community. We require contributors to sign our Contributor License Agreement, and we don't seem to have you on file. Continue to work with us on the review and improvements in this PR, and submit the signed CLA to cla@trino.io. Photos, scans, or digitally-signed PDF files are all suitable. Processing may take a few days. The CLA needs to be on file before we merge your changes. For more information, see https://github.com/trinodb/cla

i-93 · 2024-01-28T16:49:49Z

@findinpath, @alexjo2144, @electrum Does anybody have any concerns about merging this? It has been tested in our preprod environment, addressed the #20344 and didn't show any side effects.

alexjo2144

Sorry for the delay on this. Generally this seems fine to me, one small nit pick on the interface changes.

Besides that, have you tried adding a test? There are some existing ones in the product test suite that use symlink tables.

alexjo2144 · 2024-02-06T18:04:44Z

plugin/trino-hive/src/main/java/io/trino/plugin/hive/BackgroundHiveSplitLoader.java

+                    .anyMatch(path -> !fileStatuses.containsKey(path.path()));
+            // Invalidate the cache and reload
+            if (missing) {
+                directoryLister.invalidate(location);


I think the general approach here is fine, I would suggest changing the interfaces in a slightly different way though.

Exposing isCached via TableInvalidationCallback seems fine to me.

What I'd do differently is rather than exposing invalidate(Location) can we try adding an additional parameter to the HiveFileIterator cnstr, something like boolean invalidateCaches. That will get passed through DirectoryLister#listFilesRecursively to force a hard load when set to true?

My reason for that is you're assuming here that the cache key is on a Location, but that's an internal to the caching DirectoryListers that's not guaranteed to be stable.

Hi @alexjo2144.

I looked into your proposal to remove invalidate(Location) from the interface. That doesn't look right for me. Directory listers are chained with the delegate model, some of them are caching (by Location) ones, some are not (they have a Noop invalidate). If we remove invalidate from the interface we won't be able to push it down the chain.

We only call invalidate(location) if isCached(location) is true, so that in a way verifies that the particular directory lister supports cache by location.

What do you think?

i-93 · 2024-02-09T20:00:46Z

Sorry for the delay on this. Generally this seems fine to me, one small nit pick on the interface changes.

Besides that, have you tried adding a test? There are some existing ones in the product test suite that use symlink tables.

@alexjo2144 I have added a test case that fails on a stock code, but passes on the modified one.

i-93 · 2024-02-16T14:25:30Z

@alexjo2144, @findinpath, @electrum, @raunaqmorarka
Please let me know what I should do to have it merged.
Thanks!

i-93 marked this pull request as draft January 18, 2024 17:50

github-actions bot added tests:hive hive Hive connector labels Jan 18, 2024

i-93 force-pushed the cache-verification branch from 3b5f2e9 to abfa031 Compare January 19, 2024 13:07

i-93 marked this pull request as ready for review January 19, 2024 13:31

raunaqmorarka requested review from alexjo2144, findinpath and electrum January 23, 2024 04:16

i-93 force-pushed the cache-verification branch from abfa031 to 6e6aae8 Compare January 23, 2024 20:11

findinpath reviewed Jan 24, 2024

View reviewed changes

i-93 requested a review from findinpath January 25, 2024 15:46

i-93 closed this Jan 25, 2024

i-93 reopened this Jan 25, 2024

alexjo2144 reviewed Jan 25, 2024

View reviewed changes

i-93 force-pushed the cache-verification branch from 6e6aae8 to 6ee23db Compare January 27, 2024 12:20

i-93 force-pushed the cache-verification branch from 6ee23db to ebfa775 Compare January 27, 2024 14:53

cla-bot bot added the cla-signed label Jan 27, 2024

i-93 force-pushed the cache-verification branch from ebfa775 to 2668be9 Compare January 27, 2024 15:04

i-93 requested a review from alexjo2144 January 27, 2024 16:14

i-93 assigned i-93 and unassigned i-93 Jan 28, 2024

i-93 force-pushed the cache-verification branch 2 times, most recently from 9392592 to 954817f Compare February 6, 2024 16:57

alexjo2144 reviewed Feb 6, 2024

View reviewed changes

i-93 force-pushed the cache-verification branch 3 times, most recently from 0355a13 to 74ff57f Compare February 9, 2024 19:52

i-93 requested a review from alexjo2144 February 9, 2024 20:01

i-93 added 2 commits February 21, 2024 08:52

Verify file presence and retry

bbee2b1

Test case

d87cf74

i-93 force-pushed the cache-verification branch from 74ff57f to d87cf74 Compare February 21, 2024 14:36

findepi requested a review from sopel39 February 21, 2024 18:48

electrum approved these changes Feb 22, 2024

View reviewed changes

electrum merged commit 0324da7 into trinodb:master Feb 22, 2024
57 checks passed

github-actions bot added this to the 440 milestone Feb 22, 2024

raunaqmorarka mentioned this pull request Feb 22, 2024

Error: "Manifest file from the location [...] contains non-existent path: ..." #20344

Closed

colebow mentioned this pull request Feb 23, 2024

Add Trino 440 release notes #20797

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Verify the file presense for cached directory lister and retry #20414

Verify the file presense for cached directory lister and retry #20414

i-93 commented Jan 18, 2024

cla-bot bot commented Jan 18, 2024

cla-bot bot commented Jan 19, 2024

cla-bot bot commented Jan 19, 2024

cla-bot bot commented Jan 19, 2024

cla-bot bot commented Jan 19, 2024

mosabua commented Jan 22, 2024

mosabua commented Jan 22, 2024

cla-bot bot commented Jan 23, 2024

findinpath Jan 24, 2024

i-93 Jan 24, 2024

i-93 commented Jan 25, 2024

alexjo2144 Jan 25, 2024

i-93 Jan 25, 2024

i-93 commented Jan 26, 2024

cla-bot bot commented Jan 27, 2024

i-93 commented Jan 28, 2024

alexjo2144 left a comment

alexjo2144 Feb 6, 2024 •

edited

Loading

i-93 Feb 8, 2024

i-93 commented Feb 9, 2024

i-93 commented Feb 16, 2024

Verify the file presense for cached directory lister and retry #20414

Verify the file presense for cached directory lister and retry #20414

Conversation

i-93 commented Jan 18, 2024

Description

Release notes

cla-bot bot commented Jan 18, 2024

cla-bot bot commented Jan 19, 2024

cla-bot bot commented Jan 19, 2024

cla-bot bot commented Jan 19, 2024

cla-bot bot commented Jan 19, 2024

mosabua commented Jan 22, 2024

mosabua commented Jan 22, 2024

cla-bot bot commented Jan 23, 2024

findinpath Jan 24, 2024

Choose a reason for hiding this comment

i-93 Jan 24, 2024

Choose a reason for hiding this comment

i-93 commented Jan 25, 2024

alexjo2144 Jan 25, 2024

Choose a reason for hiding this comment

i-93 Jan 25, 2024

Choose a reason for hiding this comment

i-93 commented Jan 26, 2024

cla-bot bot commented Jan 27, 2024

i-93 commented Jan 28, 2024

alexjo2144 left a comment

Choose a reason for hiding this comment

alexjo2144 Feb 6, 2024 • edited Loading

Choose a reason for hiding this comment

i-93 Feb 8, 2024

Choose a reason for hiding this comment

i-93 commented Feb 9, 2024

i-93 commented Feb 16, 2024

alexjo2144 Feb 6, 2024 •

edited

Loading