raunaqmorarka commented on code in PR #18188:
URL: https://github.com/apache/iceberg/pull/18188#discussion_r4183977346
##########
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
+ public void testRefreshSendsIfNoneMatchAndKeepsMetadataOnNotModified() {
Review Comment:
You're right, they overlapped. I dropped this one since
`notModifiedResponseOnRefresh` covers it.
##########
core/src/test/java/org/apache/iceberg/rest/TestRESTCatalog.java:
##########
@@ -1439,6 +1442,17 @@ public void testTableAuth(
loaded.refresh(); // refresh to force reload
+ // refresh sends the ETag of the loaded metadata along with the table
headers
+ String eTag =
+ ETagProvider.of(
+ ((BaseTable) loaded).operations().current().metadataFileLocation(),
Review Comment:
Good point. The refresh checks here now use `containsHeaders`, so the
`If-None-Match` value no longer matters. The rest of the test is back to how it
is on main.
##########
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
+ public void testRefreshSendsIfNoneMatchAndKeepsMetadataOnNotModified() {
+ RESTCatalogAdapter adapter = Mockito.spy(new
RESTCatalogAdapter(backendCatalog));
+ RESTCatalog catalog = catalog(adapter);
+ if (requiresNamespaceCreate()) {
+ catalog.createNamespace(TABLE.namespace());
+ }
+
+ catalog.createTable(TABLE, SCHEMA);
+ AtomicReference<Object> lastLoadResponse = new AtomicReference<>();
+ Mockito.doAnswer(
+ invocation -> {
+ Object response = invocation.callRealMethod();
+ lastLoadResponse.set(response);
+ return response;
+ })
+ .when(adapter)
+ .execute(
+ matches(HTTPMethod.GET, RESOURCE_PATHS.table(TABLE)),
+ eq(LoadTableResponse.class),
+ any(),
+ any());
+
+ TableOperations ops = ((BaseTable) catalog.loadTable(TABLE)).operations();
+ TableMetadata loaded = ops.current();
+ String metadataLocation = loaded.metadataFileLocation();
+ // the adapter folds query params into the ETag, so the loadTable and
refresh ETags differ
+ String loadTableETag = ETagProvider.of(metadataLocation,
Map.of("snapshots", "all"));
+ String refreshETag = ETagProvider.of(metadataLocation, Map.of());
+
+ assertThat(ops.refresh()).isSameAs(loaded);
+ assertThat(lastLoadResponse.get()).isNotNull();
+ verify(adapter)
+ .execute(
+ matches(
+ HTTPMethod.GET,
+ RESOURCE_PATHS.table(TABLE),
+ Map.of(HttpHeaders.IF_NONE_MATCH, loadTableETag),
+ Map.of()),
+ eq(LoadTableResponse.class),
+ any(),
+ any());
+
+ assertThat(ops.refresh()).isSameAs(loaded);
+ assertThat(lastLoadResponse.get()).isNull();
+ verify(adapter)
+ .execute(
+ matches(
+ HTTPMethod.GET,
+ RESOURCE_PATHS.table(TABLE),
+ Map.of(HttpHeaders.IF_NONE_MATCH, refreshETag),
+ Map.of()),
+ eq(LoadTableResponse.class),
+ any(),
+ any());
+ }
+
+ @Test
+ public void testRefreshReloadsChangedTableAndAdoptsNewETag() {
+ RESTCatalogAdapter adapter = Mockito.spy(new
RESTCatalogAdapter(backendCatalog));
+ RESTCatalog catalog = catalog(adapter);
+ if (requiresNamespaceCreate()) {
+ catalog.createNamespace(TABLE.namespace());
+ }
+
+ catalog.createTable(TABLE, SCHEMA);
+ TableOperations ops = ((BaseTable) catalog.loadTable(TABLE)).operations();
+ TableMetadata loaded = ops.current();
+ String loadTableETag =
+ ETagProvider.of(loaded.metadataFileLocation(), Map.of("snapshots",
"all"));
+
+ // another client changes the table
+ backendCatalog
+ .loadTable(TABLE)
+ .updateSchema()
+ .addColumn("extra", Types.LongType.get())
+ .commit();
+
+ TableMetadata refreshed = ops.refresh();
+ assertThat(refreshed).isNotSameAs(loaded);
+
assertThat(refreshed.metadataFileLocation()).isNotEqualTo(loaded.metadataFileLocation());
+ assertThat(refreshed.schema().findField("extra")).isNotNull();
+ verify(adapter)
+ .execute(
+ matches(
+ HTTPMethod.GET,
+ RESOURCE_PATHS.table(TABLE),
+ Map.of(HttpHeaders.IF_NONE_MATCH, loadTableETag),
+ Map.of()),
+ eq(LoadTableResponse.class),
+ any(),
+ any());
+
+ String refreshETag = ETagProvider.of(refreshed.metadataFileLocation(),
Map.of());
+ assertThat(ops.refresh()).isSameAs(refreshed);
+ verify(adapter)
+ .execute(
+ matches(
+ HTTPMethod.GET,
+ RESOURCE_PATHS.table(TABLE),
+ Map.of(HttpHeaders.IF_NONE_MATCH, refreshETag),
+ Map.of()),
+ eq(LoadTableResponse.class),
+ any(),
+ any());
+ }
+
+ @Test
+ public void testRefreshUsesETagFromCommit() {
+ RESTCatalogAdapter adapter = Mockito.spy(new
RESTCatalogAdapter(backendCatalog));
+ RESTCatalog catalog = catalog(adapter);
+ if (requiresNamespaceCreate()) {
Review Comment:
Agreed. That check is gone now that the tests moved.
##########
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
+ public void testRefreshSendsIfNoneMatchAndKeepsMetadataOnNotModified() {
+ RESTCatalogAdapter adapter = Mockito.spy(new
RESTCatalogAdapter(backendCatalog));
+ RESTCatalog catalog = catalog(adapter);
+ if (requiresNamespaceCreate()) {
+ catalog.createNamespace(TABLE.namespace());
+ }
+
+ catalog.createTable(TABLE, SCHEMA);
+ AtomicReference<Object> lastLoadResponse = new AtomicReference<>();
+ Mockito.doAnswer(
+ invocation -> {
+ Object response = invocation.callRealMethod();
+ lastLoadResponse.set(response);
+ return response;
+ })
+ .when(adapter)
+ .execute(
+ matches(HTTPMethod.GET, RESOURCE_PATHS.table(TABLE)),
+ eq(LoadTableResponse.class),
+ any(),
+ any());
+
+ TableOperations ops = ((BaseTable) catalog.loadTable(TABLE)).operations();
+ TableMetadata loaded = ops.current();
+ String metadataLocation = loaded.metadataFileLocation();
+ // the adapter folds query params into the ETag, so the loadTable and
refresh ETags differ
+ String loadTableETag = ETagProvider.of(metadataLocation,
Map.of("snapshots", "all"));
+ String refreshETag = ETagProvider.of(metadataLocation, Map.of());
+
+ assertThat(ops.refresh()).isSameAs(loaded);
+ assertThat(lastLoadResponse.get()).isNotNull();
+ verify(adapter)
+ .execute(
+ matches(
+ HTTPMethod.GET,
+ RESOURCE_PATHS.table(TABLE),
+ Map.of(HttpHeaders.IF_NONE_MATCH, loadTableETag),
+ Map.of()),
+ eq(LoadTableResponse.class),
+ any(),
+ any());
+
+ assertThat(ops.refresh()).isSameAs(loaded);
+ assertThat(lastLoadResponse.get()).isNull();
+ verify(adapter)
+ .execute(
+ matches(
+ HTTPMethod.GET,
+ RESOURCE_PATHS.table(TABLE),
+ Map.of(HttpHeaders.IF_NONE_MATCH, refreshETag),
+ Map.of()),
+ eq(LoadTableResponse.class),
+ any(),
+ any());
+ }
+
+ @Test
+ public void testRefreshReloadsChangedTableAndAdoptsNewETag() {
+ RESTCatalogAdapter adapter = Mockito.spy(new
RESTCatalogAdapter(backendCatalog));
+ RESTCatalog catalog = catalog(adapter);
+ if (requiresNamespaceCreate()) {
+ catalog.createNamespace(TABLE.namespace());
+ }
+
+ catalog.createTable(TABLE, SCHEMA);
+ TableOperations ops = ((BaseTable) catalog.loadTable(TABLE)).operations();
+ TableMetadata loaded = ops.current();
+ String loadTableETag =
+ ETagProvider.of(loaded.metadataFileLocation(), Map.of("snapshots",
"all"));
+
+ // another client changes the table
+ backendCatalog
+ .loadTable(TABLE)
+ .updateSchema()
+ .addColumn("extra", Types.LongType.get())
+ .commit();
+
+ TableMetadata refreshed = ops.refresh();
+ assertThat(refreshed).isNotSameAs(loaded);
+
assertThat(refreshed.metadataFileLocation()).isNotEqualTo(loaded.metadataFileLocation());
+ assertThat(refreshed.schema().findField("extra")).isNotNull();
+ verify(adapter)
+ .execute(
+ matches(
+ HTTPMethod.GET,
+ RESOURCE_PATHS.table(TABLE),
+ Map.of(HttpHeaders.IF_NONE_MATCH, loadTableETag),
+ Map.of()),
+ eq(LoadTableResponse.class),
+ any(),
+ any());
+
+ String refreshETag = ETagProvider.of(refreshed.metadataFileLocation(),
Map.of());
+ assertThat(ops.refresh()).isSameAs(refreshed);
+ verify(adapter)
+ .execute(
+ matches(
+ HTTPMethod.GET,
+ RESOURCE_PATHS.table(TABLE),
+ Map.of(HttpHeaders.IF_NONE_MATCH, refreshETag),
+ Map.of()),
+ eq(LoadTableResponse.class),
+ any(),
+ any());
+ }
+
+ @Test
+ public void testRefreshUsesETagFromCommit() {
+ RESTCatalogAdapter adapter = Mockito.spy(new
RESTCatalogAdapter(backendCatalog));
+ RESTCatalog catalog = catalog(adapter);
+ if (requiresNamespaceCreate()) {
+ catalog.createNamespace(TABLE.namespace());
+ }
+
+ catalog.createTable(TABLE, SCHEMA);
+ Table table = catalog.loadTable(TABLE);
+ TableOperations ops = ((BaseTable) table).operations();
+
+ table.updateProperties().set("key", "value").commit();
+ TableMetadata committed = ops.current();
+ String commitETag =
Review Comment:
Nice idea, done. `refreshUsesETagFromCommit` captures the ETag from the
commit response and checks that the next refresh sends the same value.
##########
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
+ public void testRefreshSendsIfNoneMatchAndKeepsMetadataOnNotModified() {
+ RESTCatalogAdapter adapter = Mockito.spy(new
RESTCatalogAdapter(backendCatalog));
+ RESTCatalog catalog = catalog(adapter);
+ if (requiresNamespaceCreate()) {
+ catalog.createNamespace(TABLE.namespace());
+ }
+
+ catalog.createTable(TABLE, SCHEMA);
+ AtomicReference<Object> lastLoadResponse = new AtomicReference<>();
+ Mockito.doAnswer(
+ invocation -> {
+ Object response = invocation.callRealMethod();
+ lastLoadResponse.set(response);
+ return response;
+ })
+ .when(adapter)
+ .execute(
+ matches(HTTPMethod.GET, RESOURCE_PATHS.table(TABLE)),
+ eq(LoadTableResponse.class),
+ any(),
+ any());
+
+ TableOperations ops = ((BaseTable) catalog.loadTable(TABLE)).operations();
+ TableMetadata loaded = ops.current();
+ String metadataLocation = loaded.metadataFileLocation();
+ // the adapter folds query params into the ETag, so the loadTable and
refresh ETags differ
+ String loadTableETag = ETagProvider.of(metadataLocation,
Map.of("snapshots", "all"));
+ String refreshETag = ETagProvider.of(metadataLocation, Map.of());
+
+ assertThat(ops.refresh()).isSameAs(loaded);
+ assertThat(lastLoadResponse.get()).isNotNull();
+ verify(adapter)
+ .execute(
+ matches(
+ HTTPMethod.GET,
+ RESOURCE_PATHS.table(TABLE),
+ Map.of(HttpHeaders.IF_NONE_MATCH, loadTableETag),
+ Map.of()),
+ eq(LoadTableResponse.class),
+ any(),
+ any());
+
+ assertThat(ops.refresh()).isSameAs(loaded);
+ assertThat(lastLoadResponse.get()).isNull();
+ verify(adapter)
+ .execute(
+ matches(
+ HTTPMethod.GET,
+ RESOURCE_PATHS.table(TABLE),
+ Map.of(HttpHeaders.IF_NONE_MATCH, refreshETag),
+ Map.of()),
+ eq(LoadTableResponse.class),
+ any(),
+ any());
+ }
+
+ @Test
+ public void testRefreshReloadsChangedTableAndAdoptsNewETag() {
+ RESTCatalogAdapter adapter = Mockito.spy(new
RESTCatalogAdapter(backendCatalog));
+ RESTCatalog catalog = catalog(adapter);
+ if (requiresNamespaceCreate()) {
+ catalog.createNamespace(TABLE.namespace());
+ }
+
+ catalog.createTable(TABLE, SCHEMA);
+ TableOperations ops = ((BaseTable) catalog.loadTable(TABLE)).operations();
+ TableMetadata loaded = ops.current();
+ String loadTableETag =
+ ETagProvider.of(loaded.metadataFileLocation(), Map.of("snapshots",
"all"));
+
+ // another client changes the table
+ backendCatalog
+ .loadTable(TABLE)
+ .updateSchema()
+ .addColumn("extra", Types.LongType.get())
+ .commit();
+
+ TableMetadata refreshed = ops.refresh();
+ assertThat(refreshed).isNotSameAs(loaded);
+
assertThat(refreshed.metadataFileLocation()).isNotEqualTo(loaded.metadataFileLocation());
+ assertThat(refreshed.schema().findField("extra")).isNotNull();
+ verify(adapter)
+ .execute(
+ matches(
+ HTTPMethod.GET,
+ RESOURCE_PATHS.table(TABLE),
+ Map.of(HttpHeaders.IF_NONE_MATCH, loadTableETag),
+ Map.of()),
+ eq(LoadTableResponse.class),
+ any(),
+ any());
+
+ String refreshETag = ETagProvider.of(refreshed.metadataFileLocation(),
Map.of());
+ assertThat(ops.refresh()).isSameAs(refreshed);
+ verify(adapter)
+ .execute(
+ matches(
+ HTTPMethod.GET,
+ RESOURCE_PATHS.table(TABLE),
+ Map.of(HttpHeaders.IF_NONE_MATCH, refreshETag),
+ Map.of()),
+ eq(LoadTableResponse.class),
+ any(),
+ any());
+ }
+
+ @Test
+ public void testRefreshUsesETagFromCommit() {
+ RESTCatalogAdapter adapter = Mockito.spy(new
RESTCatalogAdapter(backendCatalog));
+ RESTCatalog catalog = catalog(adapter);
+ if (requiresNamespaceCreate()) {
+ catalog.createNamespace(TABLE.namespace());
+ }
+
+ catalog.createTable(TABLE, SCHEMA);
+ Table table = catalog.loadTable(TABLE);
+ TableOperations ops = ((BaseTable) table).operations();
+
+ table.updateProperties().set("key", "value").commit();
+ TableMetadata committed = ops.current();
+ String commitETag =
+ ETagProvider.of(committed.metadataFileLocation(), Map.of("snapshots",
"all"));
+
+ assertThat(ops.refresh()).isSameAs(committed);
+ verify(adapter)
+ .execute(
+ matches(
+ HTTPMethod.GET,
+ RESOURCE_PATHS.table(TABLE),
+ Map.of(HttpHeaders.IF_NONE_MATCH, commitETag),
+ Map.of()),
+ eq(LoadTableResponse.class),
+ any(),
+ any());
+ }
+
+ @Test
+ public void testRefreshWithoutETagFromServer() {
+ RESTCatalogAdapter adapter = Mockito.spy(new
RESTCatalogAdapter(backendCatalog));
+ // the server answers loadTable without an ETag
+ Mockito.doAnswer(
+ invocation -> {
+ Consumer<Map<String, String>> responseHeaders =
invocation.getArgument(3);
+ Consumer<Map<String, String>> withoutETag =
+ headers -> {
+ Map<String, String> filtered = Maps.newHashMap(headers);
+ filtered.remove(HttpHeaders.ETAG);
+ responseHeaders.accept(filtered);
+ };
+ return adapter.execute(
+ invocation.getArgument(0),
+ LoadTableResponse.class,
+ invocation.getArgument(2),
+ withoutETag,
+ ParserContext.builder().build());
+ })
+ .when(adapter)
+ .execute(
+ matches(HTTPMethod.GET, RESOURCE_PATHS.table(TABLE)),
+ eq(LoadTableResponse.class),
+ any(),
+ any());
+
+ RESTCatalog catalog = catalog(adapter);
+ if (requiresNamespaceCreate()) {
+ catalog.createNamespace(TABLE.namespace());
+ }
+
+ catalog.createTable(TABLE, SCHEMA);
+ Table table = catalog.loadTable(TABLE);
+ TableOperations ops = ((BaseTable) table).operations();
+ TableMetadata loaded = ops.current();
+
+ assertThat(ops.refresh()).isSameAs(loaded);
+
+ // the commit response carries an ETag, but the refresh response after it
does not
+ table.updateProperties().set("key", "value").commit();
Review Comment:
Thanks for the outline, I followed it. The test is now `refreshWithoutETag`,
with no commit step. It checks that refresh sends no `If-None-Match` and keeps
the loaded metadata.
--
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]
