Invalidate empty value metastore cache entry for a non-existent table

[nmahadevuni]

Description

If we query a table that is non-existent, we store [Key=Table, Value=Optional.Empty] metastore cache entry. If the table is created outside Presto, we keep throwing "Table does not exist" until refresh time is reached. Invalidating this empty value to fix this.

Motivation and Context

Changes to tables by different tools is common in disaggregated query engines. This fix tries to solve a possible stale cache entry case when a table is created outside of Presto, after querying the same non-existent table once from Presto.

Impact

No impact

Test Plan

No new tests added.

== NO RELEASE NOTE ==

[pratyakshsharma]

Thank you for the fix @nmahadevuni. Just wondering which one of the below should be a more ideal way to solve this -

1. Not put any entry in the cache when the table does not exist
2. Force a refresh like this fix.

I think option 1 would involve only one get call from the cache and one fetch from metastore when retrieving the details about the table as compared to option 2 where we execute two get calls from cache and one fetch from metastore. Thoughts?

[nmahadevuni]

Thank you for the fix @nmahadevuni. Just wondering which one of the below should be a more ideal way to solve this -

1. Not put any entry in the cache when the table does not exist
2. Force a refresh like this fix.

I think option 1 would involve only one get call from the cache and one fetch from metastore when retrieving the details about the table as compared to option 2 where we execute two get calls from cache and one fetch from metastore. Thoughts?

Thanks, Pratyaksh.

1. Tried doing this. put method is deep inside CacheLoader functional interface, which requires a big change to override.
2. We force a refresh only for a case where table doesn't exist. These cases should be very less in a real workload.

Another solution is we can invalidate the entry if we get a empty object, but I thought refresh would be a better way to handle this, since if any other session is trying to load the table in to cache, then this refresh would be a no-op.

[pratyakshsharma]

Thank you for the analysis Naveen.
I was not intending to make changes in the library, but just put a check for empty object at the place where the write flow of cache loader library gets triggered from presto codebase.

I am actually a bit more inclined towards adding a check for empty object as an alternative for the reasons mentioned in my previous comment. Also, the chances of concurrent sessions trying to deal with the same non existent table are pretty less I think.

We can probably wait to hear from one of the committers if you think otherwise.

[nmahadevuni]

Thank you for the analysis Naveen. I was not intending to make changes in the library, but just put a check for empty object at the place where the write flow of cache loader library gets triggered from presto codebase.

I am actually a bit more inclined towards adding a check for empty object as an alternative for the reasons mentioned in my previous comment. Also, the chances of concurrent sessions trying to deal with the same non existent table are pretty less I think.

We can probably wait to hear from one of the committers if you think otherwise.

Yeah, that loader call happens in the third party library, we cannot intercept that, we can intercept only in our value loader method, but cannot influence the put logic.

[pratyakshsharma]

My bad, on taking a closer look at the code, I get what you are saying. Even the computation of the value to be put in the cache happens inside the guava library. The current fix makes sense and looks good to me.

[agrawalreetika]

@nmahadevuni Thanks for making the changes.
Can we write a product test case for this scenario, where we can try to query non-existing table from Presto first and then create the same table created via hive?

[imjalpreet]

nit: Just thinking out loud, should we invalidate the entry instead of refreshing it? While this may not lead to a significant improvement, invalidating the entry—rather than caching an empty table—could save an extra call to the metastore when the table is empty the very first time. Since we plan to call the metastore for any cached empty table, it might be more efficient not to cache it at all. This approach would also provide more accurate JMX metrics. If we continue to cache non-existent tables, the count of actual cached tables may not be accurate since these empty table entries would also be counted.

[pratyakshsharma]

I am aligned with this.

[nmahadevuni]

Have changed it to invalidate the entry.

[nmahadevuni]

@nmahadevuni Thanks for making the changes. Can we write a product test case for this scenario, where we can try to query non-existing table from Presto first and then create the same table created via hive?

I have added unit tests in TestInMemoryCachingHiveMetastore.java which simulates this scenario.

[nmahadevuni]

@imjalpreet @agrawalreetika @pratyakshsharma Thank you. Addressed your comments. Please review. @ZacBlanco Can you please review this?

[ZacBlanco]

I feel like we just shouldn't store the reference to the table in the cache in the first place rather than invalidating it here. I think I would actually prefer throwing a TableNotFoundException in the cache loader than just storing Optional.empty if this is the desired behavior. These semantics make the behavior of the cache non-obvious to future readers since the logic is split among two locations