Revert "Add support for Filelist caching for symlink tables"

[elharo]

Reverts #19145 due to flakiness. This PR depends on the PR that introduced the flakiness so roll this one back first.

[tdcmeehan]

Why not just disable the product test?

[elharo]

This is the only test for the functionality that was added. Given the state it's in, I'm not confident the feature works. Rolling this back is the safest, most conservative option.

[tdcmeehan]

@elharo based on the stack trace in #22425, it seems to be failing during the setup portion of the test case. To me it seems like a testing issue, and it would be quicker and more straightforward to just disable the test. Even if you're correct, the procedure that was added is opt-in, and no one will be using this until 0.287 is released, and that release hasn't even been cut yet, so there's actually only a risk if someone is using recent unstable releases (which are not widely used).

[elharo]

The way I think about it is this. If I reviewed the original PR, minus the test, then the first thing I would ask is that a test be added. Without the test I can't be sure this function actually works as described. If this had been in production for a while, then maybe, but since it hasn't both test and model code should be reverted until a new test can be written that shows this works.

[agrawalreetika]

Hi @elharo @tdcmeehan,
I have been traveling, but I doubt if the feature is actually affected. I can take out some time and verify the feature if that works and we can disable the tests for now. As I checked the last some recent changes caused this failing and affecting JMX metrics in here.
LMK what do you guys think?

[elharo]

I'm not comfortable with an in-between state where we leave the feature in untested. The safe options are fix or revert. If you can find and revert some recent change that caused this to start failing, that would be great. Otherwise, let's revert now and fix it at our leisure later.

[tdcmeehan]

@elharo like I said, because this is an opt-in feature (it's a procedure call that's not in any of our public documentation yet), and it's only in unstable releases, my conclusion is that it is safe, because I trust that @agrawalreetika will root cause prior to the release. I've raised #22449.

[agrawalreetika]

@elharo Could you please confirm if we are seeing test failure for https://github.com/prestodb/presto/blob/master/presto-product-tests/src/main/java/com/facebook/presto/tests/hive/TestSymlinkTableListCaching.java as well? I didn't find any CI example with failure on this.

[elharo]

No, I don't think TestSymlinkTableListCaching is flaky but it depends on the PR that is.

Are you proposing to block the release until this is fixed? That feels risky. It seems much safer to simply roll everything back, and then fix at our leisure. If the fix is done before the release, great. If not, nothing else needs to wait for it.

[agrawalreetika]

@elharo Sure thanks for confirmation. These changes shouldn't be flaky.

Also for other one, I have found the RCA for flakiness for TestDirectoryListCacheInvalidation and proposed the test modification #22455

[tdcmeehan]

Superseded by #22455