gaborkaszab commented on code in PR #18188:
URL: https://github.com/apache/iceberg/pull/18188#discussion_r4092187758
##########
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:
I'm not sure why we introduce a function that is deprecated on creation. I
recall there might be users overriding this, but technically just adding a new
param to a protected method is not breaking the API, right?
##########
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:
Why is this a TreeMap? I recall elsewhere we used HashMap for response
headers.
##########
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:
Can this simple be `readHeaders()`? whenever we add a new stuff there we
don't want to extend the name with the new param.
##########
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:
I had the impression that all the different variations of the constructors
call the same one that actually constructs the object. Shouldn't all the other
constructors add the extra null param for the ETag? That way we could avoid a
chain of constructor calls that it's not that easy to maintain or follow.
##########
core/src/main/java/org/apache/iceberg/rest/RESTTableOperations.java:
##########
@@ -222,9 +286,17 @@ public void commit(TableMetadata base, TableMetadata
metadata) {
// the error handler will throw necessary exceptions like
CommitFailedException and
// UnknownCommitStateException
// TODO: ensure that the HTTP client lib passes HTTP client errors to the
error handler
+ Map<String, String> responseHeaders =
Maps.newTreeMap(String.CASE_INSENSITIVE_ORDER);
Review Comment:
Same TreeMap comment
##########
core/src/main/java/org/apache/iceberg/rest/RESTTableOperations.java:
##########
@@ -315,6 +387,8 @@ private TableMetadata
updateCurrentMetadata(LoadTableResponse response) {
this.current = checkUUID(current, response.tableMetadata());
}
+ this.eTag = responseETag;
Review Comment:
Shouldn't we conditionally set the ETag as we do for setting `current`
inside the if? Shouldn't they move together?
##########
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:
Previously we called the `client.get()` variation that received a Supplier
for headers and now we switch to another variation that receives a supplied
map. I'd prefer to not do that switch and use the one with Supplier.
--
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]
