Core: Freshness-aware table loading in REST catalog

[gaborkaszab]

This is the client-side improvement for the freshness-aware table
loading in REST catalog. The main design is the following:
- REST server can send an ETag with the LoadTableResponse
- The client can use this ETag to populate the IF_NONE_MATCH header
with the next loadTable request
- The server can send a 304-NOT_MODIFIED response without a body if
the table has not been changed based on the ETag
- The client when receives a 304, then returns the latest table
object associated with the ETag from cache

[gaborkaszab]

This change has 2 dependencies also included in the chain:

1. Core: Reference IRC to return 304-NotModified #14035 for the IRC to return 304
2. Core: Add RESTCatalog tests for FileIOTracker #14378 For FileIOTracker tests

[gaborkaszab]

Let me give an initial insight into the different decisions I made during the implementation:

1) Client-side table cache

Entire table objects are cached on the client side. The table objects hold user level configs and credentials, so in order to not share these accross users, the client-side table cache is arranged on a per-sessionID basis. For simplicity I chose to use a composite key: (SessionID, TableIdentifier) for the cache.

Later if there is a need, the cache can be enhanced to have 2 separate levels for the sessionID and the TableIdentifier instead of a composite key. With this we can have different cache eviction policies for the sessions (max number if entries, TTL) and for the tables. Also we could separate sessions not to evict tables related to other sessions.
For a reference implementation see RESTTableCache here.

2) Table objects returned by loadTable

Currently, each RESTCatalog.loadTable() call returns a different Table object. Changing this and returning 'shared' table objects similarly to CachingCatalog is a breaking change in my opinion. Take this use-case: One query could load the table for query planning, then another table also loads the table and commits some changes. In this case the table being held by the first query is changed in the meantime, and as it's a shared object it could mess up planning.
This is why I chose not to return the cached Table objects directly, but to do a 'clone' object instead and return that. As a result, in case the client commits to the table, it won't be reflected in the cache and the next loadTable request to the server will result in a full table load, because the ETag stored on the client side is not the latest one.

3) Table commits

As a direct result of the above point the table commits through RESTTableOperations aren't reflected in the cache. As a future improvement we can enhance this, but I haven't found a very clean solution for this. What I experimented with is to provide a loadTableCallback to the RESTTableOperations that is called after getting the LoadTableResult. Setting up such a callback properly can update the cache in RESTSessionCatalog.
For a reference implementation see RESTTableOperations and RESTSessionCatalog.loadTableCallback() here.

4) SnapshotMode = REFS

When lazy snapshot loading is configured, loadTable will populate the cache with a table object that has partially loaded snapshot list. If the client calls snapshots() and loads the rest of the snapshots, this won't be reflected in the cache. As a future improvement the snapshotsSupplier provided to TableMetadata could be clever enough to refresh the cache too. I found thes too complicated to implement for the initial version.

5) Place of the ETag

Initially I've put the ETag into RESTTableOperations, but since the operations doesn't participate in the cache maintenance nor in freshness-aware loading, then I decided not to put ETag there. TableMetadata seemed too general, I wanted to use a REST catalog specific location, hence I've introduced a composite value for the table cache TableWithETag.

6) Cache eviction policies

No weakKeys, weakValues (or similar) could be used with the current design: the returned tables are clones, so weakValues would nearly immediately evict the entry from the cache right after adding it.
The maximum number of entries is controlled via a catalog property. With the (SessionID, TableIdentifier) composite key this means that one active user could evict the tables from a less active user. For future improvements see section 1)
Another way of evicting from cache is using a TTL that is also configurable by a catalog property. Note, the timer resets after write and not after access. The reason is that when loading the table into the cache will also contain configs and credentials that could expire. Setting a TTL after write properly could evict the table from cache before the expiration of such configs and credentials.

[XJDKC]

This approach requires the table to have an entry in the cachedTable, so that we can retrieve its ETag and include it in the loadTable request. However, what if the ETag is cached externally, and the caller wants to supply the ETag directly when loading the table?

This use case is valid and common, the caller may store the ETag in somewhere for performance optimizations and expect to use it in the next loadTable request (may not use the same client).

[gaborkaszab]

Thanks for taking a look, @XJDKC !
External ETags weren't in the scope of the original design. In fact the ETags for this functionality are deliberately kept 'under the hood' and not exposed to the clients/engines.
The use-case you describe wouldn't simply require the ETag to be passed in to loadTable() but also to have some mechanism to answer with a table object in case of a 304-NotModified. Would you simply expect null from loadTable() and have a cache on the caller side to answer such queries?

[XJDKC]

Yeah, the caller might maintain a cache on the caller side. Many catalogs now support multiplexing, so the caller may have a cache to store the latest table metadata info.

For this use case, if the caller got an null pointer or something that indicates that the table is not modified, the caller needs to handle it by itself. e.g., don't need to proceed.

Does it make sense to allow the caller to pass in the ETag in the SessionContext? Maybe via a property in the property map?

(Noticed that, unfortunately, for RESTCatalog::loadTable, it doesn't allow the caller to pass in a SessionContext at table level, is it possible to add another interface?)

[gaborkaszab]

Thank you for the explanation, @XJDKC !
I see your point. At the moment ETag handling, and answering from cache is deliberately kept internal to RESTSessionCatalog. One of the requirements when designing this was to not change existing interfaces or introduce new ones. Maybe we can re-visit this later on, but for simplicity and keeping the API clean I don't think we should provide a way to provide external ETags through the loadTable API
Additional observation I have is ETags are coming from the REST server, and are kept within the RESTSessionCatalog, but not persisted into the table object, nor into the table ops, so the users of loadTable API won't have a way to get the ETag of a table other than calculating it themselves. However, that goes against the purpose of an ETag, as it should be opaque to the clients.

[XJDKC]

I fully understand your points, they make sense!
But just to add some context from real-world use cases: There are scenarios where the query engine tries to optimize loadTable performance by persisting the ETag somewhere. That way, when the engine restarts, it doesn't have to perform a full loadTable again and can instead reuse the previously fetched version.

It would be great if we could provide some flexibility or hooks for customization when implementing this!

[pvary]

Could these users call the REST catalog API directly?

[gaborkaszab]

Thanks you for describing the use case @XJDKC !
When the engine restarts, it's not enough to persist the ETag somewhere because in order to answer a loadTable call you need a table object too.
Anyway, I still think that the scope of this PR should be as narrow as possible, and not exposing ETags is what we agreed on in the proposal doc. Once this work is done, further changes could be initiated.

[gaborkaszab]

Last change is a rebase with main

[gaborkaszab]

Hi @danielcweeks @nastra @amogh-jahagirdar ,
This is the client-side of the freshness-aware loading improvement in REST catalog. Would you mind taking a look?

[gaborkaszab]

Hi @danielcweeks @amogh-jahagirdar @nastra ,
This one would be the finishing step for the freshness-aware loading functionality in REST catalog. Would you mind taking a look? I described a number of decision points and the reason for my decisions in one of the first comments of this PR.

[pvary]

LGTM, please rebase.
Let's see if @danielcweeks, @nastra or @amogh-jahagirdar has any more comments

[gaborkaszab]

Thanks for taking a look @pvary !
This apparently collided with the injectable rest ops PR. Resolved the conflicts.

[gaborkaszab]

Conflicts with some of the recent changes around this area. Will rebase once this PR is merged.

[gaborkaszab]

Rebased with main but apparently has another merge conflict. This time with the server-side planning changes

[gaborkaszab]

Hey @singhpk234 ,
Let me invite you to review this PR, because I saw you had an overlapping change on this area with introducing the concept of RESTTable and server-side planning. I'm resolving merge conflicts here in the code and it made me wondering what should be the strategy for interworking the two functionalities: server-side planning and freshness-aware loading .

If I'm not mistaken, RESTTable is also a regular table like BaseTable, the difference is that some scan APIs are overridden to go to a different route when planning a query, right? Following the logic, it would still make sense to have ETags for such tables and cache them with freshness-aware loading functionality.

Would be great to hear your take on this! Thanks!
(I'll come back with the resolved code to work with both features soon)

[gaborkaszab]

I managed to merge the 2 functionalities and resolve conflicts. I had to make seem adjustments. to your implementation though. E.g. RESTClient is one of the inputs for RESTTable, however, when doing my cloning of the table from cache, the client is not available only through RESTTableOps. Hence I had to give extra visibility to that member.
I also had to rewrite some of the code that returns RESTTable to handle the cloning from cache.
I'd appreciate if you could take a look @singhpk234

[gaborkaszab]

@XJDKC @flyrain After I rebased my changes I observed that this freshness-aware loading functionality messes up your injectable table ops change. The reason in a nutshell is when I return a table from client-side cache, I have to clone it (in order to the clients not having a race condition on using the same table object), but when cloning the table the injected ops is lost.
As a solution, I introduced another variant of the newTableOps that is basically to clone on ops object into another one. With this design you'll have to override this new function too, whenever this PR is merged. I haven't found any other good way to solve this.

Would you mind taking a look?

[gaborkaszab]

There is a filing test in TestRESTScanPlanning, but that apparently fails across other PRs too. cc @singhpk234

[singhpk234]

Thanks @gaborkaszab i am not sure what is causing the flakyness as my CI green when we merged, i will take a deeper look and put a fix ASAP.

[gaborkaszab]

Update on the latest changes:
There have been multiple merged PRs recently that conflicted with this one: injectable RESTTableOperations and server-side planning. In fact because of these features the freshness-aware loading code had to be revised to support interworking with the mentioned functionalities. As a result, the table cache doesn't hold actual table objects, but table suppliers, that can be used to produce table objects in case of a 304. With this, encapsulating TableOps and RESTTable creation into this table supplier we won't break injectable ops, and we can use freshness-aware loading for RESTTables too.
Thanks @pvary for the help here!

As a side-effect, since we don't cache table (including ops and IO), the FileIOTracker tests are no longer needed within this PR. Removed them.

cc @flyrain @XJDKC @singhpk234 since this intersects with the PRs you've merged recently.

[pvary]

Looks nice @gaborkaszab!

Do we have tests for how the injectable table ops change interacts with the cache?

[gaborkaszab]

Thanks for taking a look, @pvary !

Do we have tests for how the injectable table ops change interacts with the cache?
Apparently we don't. Added a new test to cover this.

[XJDKC]

This is great! Thanks @gaborkaszab and sorry that my merge ended up conflicting with your changes and require some rework on your end.