raunaqmorarka commented on code in PR #18188:
URL: https://github.com/apache/iceberg/pull/18188#discussion_r4183975693
##########
core/src/main/java/org/apache/iceberg/rest/RESTTableOperations.java:
##########
@@ -165,13 +222,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()));
+ this::readHeaders,
+ ErrorHandlers.tableErrorHandler(),
+ responseHeaders::putAll);
+
+ if (response == null) {
+ // metadata is current
+ return current;
+ }
+
+ return updateCurrentMetadata(response,
responseHeaders.get(HttpHeaders.ETAG));
+ }
+
+ private Map<String, String> readHeaders() {
+ Map<String, String> headers = readHeaders.get();
+ if (eTag == null) {
+ return headers;
+ }
+
+ Map<String, String> conditionalHeaders = Maps.newLinkedHashMap(headers);
Review Comment:
Good call, renamed to `extendedReadHeaders`.
##########
core/src/main/java/org/apache/iceberg/rest/RESTTableOperations.java:
##########
@@ -222,9 +299,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);
LoadTableResponse response;
try {
- response = client.post(path, request, LoadTableResponse.class,
mutationHeaders, errorHandler);
+ response =
Review Comment:
Makes sense. I've widened the PR title and commit message to "REST: Add
freshness-aware loading to RESTTableOperations".
##########
core/src/test/java/org/apache/iceberg/rest/TestFreshnessAwareLoading.java:
##########
@@ -285,6 +285,54 @@ public void notModifiedResponse() {
any());
}
+ @Test
+ public void notModifiedResponseOnRefresh() {
+ restCatalog.createNamespace(TABLE.namespace());
+ restCatalog.createTable(TABLE, SCHEMA);
+ BaseTable table = (BaseTable) restCatalog.loadTable(TABLE);
+ TableMetadata loaded = table.operations().current();
+
+ // the adapter hashes query params into the ETag, so the first refresh
gets a full response
Review Comment:
Good question. The difference was the `snapshots` param. `loadTable` sends
`snapshots=all`, while `refresh()` omits it. The test adapter hashed those
params into the ETag, so the two never matched. Both requests return the same
content, so I updated the adapter to treat a missing `snapshots` as `all`. The
first refresh after `loadTable` now gets a 304. Only
`snapshot-loading-mode=refs` still does a full first refresh, since its content
differs.
##########
core/src/test/java/org/apache/iceberg/rest/TestReferencedByQueryParam.java:
##########
@@ -241,12 +243,20 @@ public void refreshKeepsSendingReferencedBy() {
table.refresh();
// refresh() also runs on stale reads and commit retries, and sends no
snapshots parameter
+ String eTag =
Review Comment:
Agreed, that was brittle. I restored this test to how it is on main. I also
added a `containsHeaders` overload to `RequestMatcher` that checks the query
params and only the headers the test names. The test no longer depends on the
ETag.
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -4098,6 +4103,209 @@ public void testLoadTableWithSpecialChars() {
assertThat(metadataFileLocation).contains("ns 1 ?=-+/ns 2 ?=-+/table 1
?=-+");
}
+ @Test
Review Comment:
Agreed, moved them to `TestFreshnessAwareLoading`.
--
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]
