raunaqmorarka commented on code in PR #18188:
URL: https://github.com/apache/iceberg/pull/18188#discussion_r4093887554
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -1349,6 +1352,34 @@ protected RESTTableOperations newTableOps(
Map.of());
}
+ /**
+ * Create a new {@link RESTTableOperations} instance for simple table
operations.
+ *
+ * @deprecated since 1.13.0, will be removed in 1.14.0; use {@link
#newTableOps(RESTClient,
+ * String, Supplier, Supplier, FileIO, TableMetadata, String, Set, Map)}
instead.
+ */
+ @Deprecated
+ protected RESTTableOperations newTableOps(
Review Comment:
The 8-arg overload came in #18015 and is in the 1.12.0 RCs. If this lands in
1.13.0, changing its parameters breaks subclasses that override or call it, so
this follows the deprecate-and-delegate step #18015 used for the 7-arg one.
Will this PR make the next 1.12.0 RC? If so, I'll change the 8-arg overload in
place instead.
##########
core/src/main/java/org/apache/iceberg/rest/RESTTableOperations.java:
##########
@@ -116,6 +119,19 @@ enum UpdateType {
TableMetadata current,
Set<Endpoint> endpoints,
Map<String, String> readQueryParams) {
+ this(client, path, readHeaders, mutationHeaders, io, current, null,
endpoints, readQueryParams);
Review Comment:
Done. Every constructor now calls the full one directly.
##########
core/src/main/java/org/apache/iceberg/rest/RESTTableOperations.java:
##########
@@ -165,13 +209,33 @@ public TableMetadata current() {
@Override
public TableMetadata refresh() {
Endpoint.check(endpoints, Endpoint.V1_LOAD_TABLE);
- return updateCurrentMetadata(
+ Map<String, String> responseHeaders =
Maps.newTreeMap(String.CASE_INSENSITIVE_ORDER);
Review Comment:
Header names are case-insensitive, and HTTP/2 sends them in lowercase.
#17979 switched the `loadTable` map from a HashMap to this same TreeMap because
a server sending `etag` never got a cached table.
##########
core/src/main/java/org/apache/iceberg/rest/RESTTableOperations.java:
##########
@@ -165,13 +209,33 @@ public TableMetadata current() {
@Override
public TableMetadata refresh() {
Endpoint.check(endpoints, Endpoint.V1_LOAD_TABLE);
- return updateCurrentMetadata(
+ Map<String, String> responseHeaders =
Maps.newTreeMap(String.CASE_INSENSITIVE_ORDER);
+ LoadTableResponse response =
client.get(
path,
readQueryParams,
LoadTableResponse.class,
- readHeaders,
- ErrorHandlers.tableErrorHandler()));
+ readHeadersWithETag(),
Review Comment:
Renamed to `readHeaders()`.
##########
core/src/main/java/org/apache/iceberg/rest/RESTTableOperations.java:
##########
@@ -165,13 +209,33 @@ public TableMetadata current() {
@Override
public TableMetadata refresh() {
Endpoint.check(endpoints, Endpoint.V1_LOAD_TABLE);
- return updateCurrentMetadata(
+ Map<String, String> responseHeaders =
Maps.newTreeMap(String.CASE_INSENSITIVE_ORDER);
+ LoadTableResponse response =
client.get(
path,
readQueryParams,
LoadTableResponse.class,
- readHeaders,
- ErrorHandlers.tableErrorHandler()));
+ readHeadersWithETag(),
Review Comment:
Switched back to the `Supplier` overload with `this::readHeaders`.
--
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]
