You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by GitBox <gi...@apache.org> on 2022/10/28 14:10:41 UTC
[GitHub] [iceberg] nastra opened a new pull request, #6073: Core: Pass purgeRequested flag to REST server
nastra opened a new pull request, #6073:
URL: https://github.com/apache/iceberg/pull/6073
The motivation behind this change is that the REST server should know whether a `purge` was requested or not, rather than having the REST client throw an error.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #6073: Core: Pass purgeRequested flag to REST server
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #6073:
URL: https://github.com/apache/iceberg/pull/6073#discussion_r1014894712
##########
core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java:
##########
@@ -799,6 +799,23 @@ public void testDropTable() {
Assert.assertFalse("Table should not exist after drop", catalog.tableExists(TABLE));
}
+ @Test
+ public void testDropTableWithPurge() {
+ C catalog = catalog();
+
+ if (requiresNamespaceCreate()) {
+ catalog.createNamespace(NS);
+ }
+
+ Assertions.assertThat(catalog.tableExists(TABLE)).isFalse();
+
+ catalog.buildTable(TABLE, SCHEMA).create();
+ Assertions.assertThat(catalog.tableExists(TABLE)).isTrue();
+
+ Assertions.assertThat(catalog.dropTable(TABLE, true)).isTrue();
+ Assertions.assertThat(catalog.tableExists(TABLE)).isFalse();
Review Comment:
This looks copied from the test above. Can you copy the assertions with context strings?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #6073: Core: Pass purgeRequested flag to REST server
Posted by GitBox <gi...@apache.org>.
nastra commented on code in PR #6073:
URL: https://github.com/apache/iceberg/pull/6073#discussion_r1008114969
##########
core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java:
##########
@@ -222,8 +222,8 @@ public static LoadTableResponse createTable(
throw new IllegalStateException("Cannot wrap catalog that does not produce BaseTable");
}
- public static void dropTable(Catalog catalog, TableIdentifier ident) {
- boolean dropped = catalog.dropTable(ident);
+ public static void dropTable(Catalog catalog, TableIdentifier ident, boolean purge) {
+ boolean dropped = catalog.dropTable(ident, purge);
Review Comment:
I think we have a discrepancy in behavior between [Catalog.dropTable(ident)](https://github.com/apache/iceberg/blob/master/api/src/main/java/org/apache/iceberg/catalog/Catalog.java#L295) (purges by default) and [SessionCatalog.dropTable(..)](https://github.com/apache/iceberg/blob/master/api/src/main/java/org/apache/iceberg/catalog/SessionCatalog.java#L179-L188) (does not purge by default and therefore also has `SessionCatalog.purgeTable(..)`)
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #6073: Core: Pass purgeRequested flag to REST server
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #6073:
URL: https://github.com/apache/iceberg/pull/6073#discussion_r1014894952
##########
core/src/test/java/org/apache/iceberg/rest/TestHTTPClient.java:
##########
@@ -219,7 +219,7 @@ private static Item doExecuteRequest(
restClient.head(path, headers, onError);
return null;
case DELETE:
- return restClient.delete(path, Item.class, headers, onError);
+ return restClient.delete(path, Item.class, () -> headers, onError);
Review Comment:
Look good to me.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue merged pull request #6073: Core: Pass purgeRequested flag to REST server
Posted by GitBox <gi...@apache.org>.
rdblue merged PR #6073:
URL: https://github.com/apache/iceberg/pull/6073
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #6073: Core: Pass purgeRequested flag to REST server
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #6073:
URL: https://github.com/apache/iceberg/pull/6073#discussion_r1014894248
##########
core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java:
##########
@@ -222,13 +222,27 @@ public static LoadTableResponse createTable(
throw new IllegalStateException("Cannot wrap catalog that does not produce BaseTable");
}
+ /**
+ * @param catalog The catalog instance
+ * @param ident The table identifier of the table to be dropped.
+ * @deprecated Will be removed in 1.2.0, use {@link CatalogHandlers#dropTable(Catalog,
+ * TableIdentifier, boolean)}.
+ */
+ @Deprecated
Review Comment:
Rather than adding a boolean option, can we have two methods, `dropTable` and `purgeTable`?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #6073: Core: Pass purgeRequested flag to REST server
Posted by GitBox <gi...@apache.org>.
nastra commented on code in PR #6073:
URL: https://github.com/apache/iceberg/pull/6073#discussion_r1008117319
##########
core/src/test/java/org/apache/iceberg/rest/RESTCatalogAdapter.java:
##########
@@ -320,7 +321,10 @@ public <T extends RESTResponse> T handleRequest(
case DROP_TABLE:
{
- CatalogHandlers.dropTable(catalog, identFromPathVars(vars));
+ CatalogHandlers.dropTable(
+ catalog,
+ identFromPathVars(vars),
+ PropertyUtil.propertyAsBoolean(vars, "purgeRequested", false));
Review Comment:
we're passing `false` here if `purgeRequested` wasn't provided, but previously data was always purged due to `Catalog.dropTable(ident)` always using `purge=false`
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #6073: Core: Pass purgeRequested flag to REST server
Posted by GitBox <gi...@apache.org>.
nastra commented on code in PR #6073:
URL: https://github.com/apache/iceberg/pull/6073#discussion_r1008121297
##########
core/src/test/java/org/apache/iceberg/rest/TestHTTPClient.java:
##########
@@ -219,7 +219,7 @@ private static Item doExecuteRequest(
restClient.head(path, headers, onError);
return null;
case DELETE:
- return restClient.delete(path, Item.class, headers, onError);
+ return restClient.delete(path, Item.class, () -> headers, onError);
Review Comment:
not sure it's worth adding another overloaded `delete` method in the `RESTClient` just for this case
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #6073: Core: Pass purgeRequested flag to REST server
Posted by GitBox <gi...@apache.org>.
nastra commented on code in PR #6073:
URL: https://github.com/apache/iceberg/pull/6073#discussion_r1008114969
##########
core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java:
##########
@@ -222,8 +222,8 @@ public static LoadTableResponse createTable(
throw new IllegalStateException("Cannot wrap catalog that does not produce BaseTable");
}
- public static void dropTable(Catalog catalog, TableIdentifier ident) {
- boolean dropped = catalog.dropTable(ident);
+ public static void dropTable(Catalog catalog, TableIdentifier ident, boolean purge) {
+ boolean dropped = catalog.dropTable(ident, purge);
Review Comment:
@rdblue @danielcweeks I think we have a discrepancy in behavior between [Catalog.dropTable(ident)](https://github.com/apache/iceberg/blob/master/api/src/main/java/org/apache/iceberg/catalog/Catalog.java#L295) (purges by default) and [SessionCatalog.dropTable(..)](https://github.com/apache/iceberg/blob/master/api/src/main/java/org/apache/iceberg/catalog/SessionCatalog.java#L179-L188) (does not purge by default and therefore also has `SessionCatalog.purgeTable(..)`)
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #6073: Core: Pass purgeRequested flag to REST server
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #6073:
URL: https://github.com/apache/iceberg/pull/6073#discussion_r1014894879
##########
core/src/test/java/org/apache/iceberg/rest/RESTCatalogAdapter.java:
##########
@@ -320,7 +321,10 @@ public <T extends RESTResponse> T handleRequest(
case DROP_TABLE:
{
- CatalogHandlers.dropTable(catalog, identFromPathVars(vars));
+ CatalogHandlers.dropTable(
+ catalog,
+ identFromPathVars(vars),
+ PropertyUtil.propertyAsBoolean(vars, "purgeRequested", false));
Review Comment:
You mean always using `purge=true`?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #6073: Core: Pass purgeRequested flag to REST server
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #6073:
URL: https://github.com/apache/iceberg/pull/6073#discussion_r1014895276
##########
core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java:
##########
@@ -222,8 +222,8 @@ public static LoadTableResponse createTable(
throw new IllegalStateException("Cannot wrap catalog that does not produce BaseTable");
}
- public static void dropTable(Catalog catalog, TableIdentifier ident) {
- boolean dropped = catalog.dropTable(ident);
+ public static void dropTable(Catalog catalog, TableIdentifier ident, boolean purge) {
+ boolean dropped = catalog.dropTable(ident, purge);
Review Comment:
The `SessionCatalog` API requires specifying purge. But the `RESTCatalog` adapter does call `Catalog.dropTable` to have the same behavior.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #6073: Core: Pass purgeRequested flag to REST server
Posted by GitBox <gi...@apache.org>.
nastra commented on code in PR #6073:
URL: https://github.com/apache/iceberg/pull/6073#discussion_r1015101336
##########
core/src/test/java/org/apache/iceberg/catalog/CatalogTests.java:
##########
@@ -799,6 +799,23 @@ public void testDropTable() {
Assert.assertFalse("Table should not exist after drop", catalog.tableExists(TABLE));
}
+ @Test
+ public void testDropTableWithPurge() {
+ C catalog = catalog();
+
+ if (requiresNamespaceCreate()) {
+ catalog.createNamespace(NS);
+ }
+
+ Assertions.assertThat(catalog.tableExists(TABLE)).isFalse();
+
+ catalog.buildTable(TABLE, SCHEMA).create();
+ Assertions.assertThat(catalog.tableExists(TABLE)).isTrue();
+
+ Assertions.assertThat(catalog.dropTable(TABLE, true)).isTrue();
+ Assertions.assertThat(catalog.tableExists(TABLE)).isFalse();
Review Comment:
done
##########
core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java:
##########
@@ -222,13 +222,27 @@ public static LoadTableResponse createTable(
throw new IllegalStateException("Cannot wrap catalog that does not produce BaseTable");
}
+ /**
+ * @param catalog The catalog instance
+ * @param ident The table identifier of the table to be dropped.
+ * @deprecated Will be removed in 1.2.0, use {@link CatalogHandlers#dropTable(Catalog,
+ * TableIdentifier, boolean)}.
+ */
+ @Deprecated
Review Comment:
done
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #6073: Core: Pass purgeRequested flag to REST server
Posted by GitBox <gi...@apache.org>.
nastra commented on code in PR #6073:
URL: https://github.com/apache/iceberg/pull/6073#discussion_r1013124241
##########
core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java:
##########
@@ -222,8 +222,8 @@ public static LoadTableResponse createTable(
throw new IllegalStateException("Cannot wrap catalog that does not produce BaseTable");
}
- public static void dropTable(Catalog catalog, TableIdentifier ident) {
- boolean dropped = catalog.dropTable(ident);
+ public static void dropTable(Catalog catalog, TableIdentifier ident, boolean purge) {
Review Comment:
done
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] nastra commented on a diff in pull request #6073: Core: Pass purgeRequested flag to REST server
Posted by GitBox <gi...@apache.org>.
nastra commented on code in PR #6073:
URL: https://github.com/apache/iceberg/pull/6073#discussion_r1008118659
##########
core/src/main/java/org/apache/iceberg/rest/RESTSessionCatalog.java:
##########
@@ -223,8 +223,20 @@ public boolean dropTable(SessionContext context, TableIdentifier identifier) {
}
@Override
- public boolean purgeTable(SessionContext context, TableIdentifier ident) {
- throw new UnsupportedOperationException("Purge is not supported");
+ public boolean purgeTable(SessionContext context, TableIdentifier identifier) {
+ checkIdentifierIsValid(identifier);
+
+ try {
+ client.delete(
+ paths.table(identifier),
+ ImmutableMap.of("purgeRequested", "true"),
Review Comment:
that's the name of the query param according to https://github.com/apache/iceberg/blob/master/open-api/rest-catalog-open-api.yaml#L653
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] danielcweeks commented on a diff in pull request #6073: Core: Pass purgeRequested flag to REST server
Posted by GitBox <gi...@apache.org>.
danielcweeks commented on code in PR #6073:
URL: https://github.com/apache/iceberg/pull/6073#discussion_r1013080754
##########
core/src/main/java/org/apache/iceberg/rest/CatalogHandlers.java:
##########
@@ -222,8 +222,8 @@ public static LoadTableResponse createTable(
throw new IllegalStateException("Cannot wrap catalog that does not produce BaseTable");
}
- public static void dropTable(Catalog catalog, TableIdentifier ident) {
- boolean dropped = catalog.dropTable(ident);
+ public static void dropTable(Catalog catalog, TableIdentifier ident, boolean purge) {
Review Comment:
We need to maintain the original signature as well for semver and add deprecation for 1.2.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on a diff in pull request #6073: Core: Pass purgeRequested flag to REST server
Posted by GitBox <gi...@apache.org>.
rdblue commented on code in PR #6073:
URL: https://github.com/apache/iceberg/pull/6073#discussion_r1014895355
##########
core/src/test/java/org/apache/iceberg/rest/RESTCatalogAdapter.java:
##########
@@ -320,7 +321,10 @@ public <T extends RESTResponse> T handleRequest(
case DROP_TABLE:
{
- CatalogHandlers.dropTable(catalog, identFromPathVars(vars));
+ CatalogHandlers.dropTable(
+ catalog,
+ identFromPathVars(vars),
+ PropertyUtil.propertyAsBoolean(vars, "purgeRequested", false));
Review Comment:
I think this is correct for the REST API. If `purgeRequested` was not set then it was not requested. This should be consistent behavior, since purge would cause a failure before.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
[GitHub] [iceberg] rdblue commented on pull request #6073: Core: Pass purgeRequested flag to REST server
Posted by GitBox <gi...@apache.org>.
rdblue commented on PR #6073:
URL: https://github.com/apache/iceberg/pull/6073#issuecomment-1307507487
Looks good! Thanks for the updates, @nastra!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org