gaborkaszab commented on code in PR #18188:
URL: https://github.com/apache/iceberg/pull/18188#discussion_r4061423251
##########
core/src/main/java/org/apache/iceberg/rest/RESTTableOperations.java:
##########
@@ -162,16 +165,41 @@ public TableMetadata current() {
return current;
}
+ /** Seeds the ETag of the load response that produced {@link #current()}. */
+ void eTag(String newETag) {
Review Comment:
Is there an issue with providing ETag through the constructor?
I recall there have been a very similar PR before, because I remember asking
the same question :)
##########
core/src/main/java/org/apache/iceberg/rest/RESTTableOperations.java:
##########
@@ -162,16 +165,41 @@ public TableMetadata current() {
return current;
}
+ /** Seeds the ETag of the load response that produced {@link #current()}. */
+ void eTag(String newETag) {
+ this.eTag = newETag;
+ }
+
@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()));
+ refreshHeaders(),
+ ErrorHandlers.tableErrorHandler(),
+ responseHeaders::putAll);
+
+ if (response == null) {
+ // 304 Not Modified: the server confirmed the metadata behind the ETag
is still current
+ return current;
+ }
+
+ return updateCurrentMetadata(response, responseHeaders);
+ }
+
+ private Map<String, String> refreshHeaders() {
Review Comment:
Method name might be misleading, these are not just refresh headers.
##########
core/src/main/java/org/apache/iceberg/rest/RESTTableOperations.java:
##########
@@ -162,16 +165,41 @@ public TableMetadata current() {
return current;
}
+ /** Seeds the ETag of the load response that produced {@link #current()}. */
+ void eTag(String newETag) {
+ this.eTag = newETag;
+ }
+
@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()));
+ refreshHeaders(),
+ ErrorHandlers.tableErrorHandler(),
+ responseHeaders::putAll);
+
+ if (response == null) {
+ // 304 Not Modified: the server confirmed the metadata behind the ETag
is still current
Review Comment:
nit: this seems over-explained: "Metadata is current". all 304 and ETag and
others are relevant for the HTTP layer, and kind of an implementation detail
there, I don't think we should mention them in a comment here.
##########
core/src/main/java/org/apache/iceberg/rest/RESTTableOperations.java:
##########
@@ -306,7 +342,8 @@ private static Long
expectedSnapshotIdIfSnapshotAddOnly(List<MetadataUpdate> upd
return addedSnapshotId;
}
- private TableMetadata updateCurrentMetadata(LoadTableResponse response) {
+ private TableMetadata updateCurrentMetadata(
Review Comment:
We don't need all the response headers here, just the ETag
##########
core/src/main/java/org/apache/iceberg/rest/RESTTableOperations.java:
##########
@@ -315,6 +352,9 @@ private TableMetadata
updateCurrentMetadata(LoadTableResponse response) {
this.current = checkUUID(current, response.tableMetadata());
}
+ // a response without an ETag disables conditional refresh until the
server sends one again
Review Comment:
nit: irrelevant comment
--
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]
