wgtmac commented on code in PR #616: URL: https://github.com/apache/iceberg-cpp/pull/616#discussion_r3394982996
########## src/iceberg/catalog/rest/rest_catalog.cc: ########## @@ -479,6 +523,7 @@ Result<std::shared_ptr<Table>> RestCatalog::LoadTable(const TableIdentifier& ide ICEBERG_ASSIGN_OR_RAISE(const auto body, LoadTableInternal(identifier)); ICEBERG_ASSIGN_OR_RAISE(auto json, FromJsonString(body)); ICEBERG_ASSIGN_OR_RAISE(auto load_result, LoadTableResultFromJson(json)); + ICEBERG_RETURN_UNEXPECTED(RememberTableSession(identifier, load_result.config)); Review Comment: One remaining gap: this stores a table session, but refresh still won't use it. `Table::Refresh()` calls back into `RestCatalog::LoadTable()`, and `LoadTableInternal()` always sends the GET with `catalog_session_`, so the cached table session is only used by `UpdateTable()`. In Java, `RESTTableOperations.refresh()` uses the table client created from `authManager.tableSession(...)`. Could we make `LoadTableInternal()` use `SessionFor(identifier)` when a table session is already cached, falling back to `catalog_session_` for the initial load, and add a RestCatalog-level test that proves table response config/session headers are used for refresh/update? -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
