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/11/22 14:10:11 UTC
[GitHub] [iceberg] pvary opened a new pull request, #6248: Flink: Fix tests creating catalog after FLINK-29677
pvary opened a new pull request, #6248:
URL: https://github.com/apache/iceberg/pull/6248
FLINK-29677 add a check, so the currently used `Catalog` could not be dropped.
FLINK-29677 will land in Flink 1.16.1, and 1.17.0 and when we start to use it we need to update some of our tests.
This PR creates a method `pushCatalog` to store the current catalog, and `popCatalog` to restore this old catalog. This should be used to restore the original catalog before dropping the one used in the tests.
--
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 #6248: Flink: Fix tests creating catalog after FLINK-29677
Posted by GitBox <gi...@apache.org>.
nastra commented on code in PR #6248:
URL: https://github.com/apache/iceberg/pull/6248#discussion_r1031235275
##########
flink/v1.16/flink/src/test/java/org/apache/iceberg/flink/source/TestFlinkTableSource.java:
##########
@@ -97,6 +97,7 @@ public void before() {
public void clean() {
sql("DROP TABLE IF EXISTS %s.%s", DATABASE_NAME, TABLE_NAME);
sql("DROP DATABASE IF EXISTS %s", DATABASE_NAME);
+ dropCatalog(CATALOG_NAME, true);
sql("DROP CATALOG IF EXISTS %s", CATALOG_NAME);
Review Comment:
I think this line needs to be removed, given that we call dropCatalog above
##########
flink/v1.16/flink/src/test/java/org/apache/iceberg/flink/FlinkTestBase.java:
##########
@@ -113,4 +113,15 @@ protected void assertSameElements(String message, Iterable<Row> expected, Iterab
.as(message)
.containsExactlyInAnyOrderElementsOf(expected);
}
+
+ /**
+ * We can not drop currently used catalog after FLINK-29677, so we have make sure that we do not use the current
+ * catalog before dropping it. This method switches to the 'default_catalog' and drops the one requested.
+ * @param catalogName The catalog to drop
+ * @param ifExist If we should use the 'IF EXISTS' when dropping the catalog
+ */
+ protected void dropCatalog(String catalogName, boolean ifExist) {
Review Comment:
nit: ifExist -> ifExists
--
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] stevenzwu commented on pull request #6248: Flink: Fix tests creating catalog after FLINK-29677
Posted by GitBox <gi...@apache.org>.
stevenzwu commented on PR #6248:
URL: https://github.com/apache/iceberg/pull/6248#issuecomment-1326792192
thanks @pvary for the contribution and @nastra for reviewing.
--
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] stevenzwu commented on pull request #6248: Flink: Fix tests creating catalog after FLINK-29677
Posted by GitBox <gi...@apache.org>.
stevenzwu commented on PR #6248:
URL: https://github.com/apache/iceberg/pull/6248#issuecomment-1325523159
> switches to the default_catalog and then executes the DROP CATALOG <if exists> <name>
I like @nastra 's suggestion. also maybe add some code comment to explain why we need to switch the catalog before calling `DROP CATALOG ...`
--
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] pvary commented on a diff in pull request #6248: Flink: Fix tests creating catalog after FLINK-29677
Posted by GitBox <gi...@apache.org>.
pvary commented on code in PR #6248:
URL: https://github.com/apache/iceberg/pull/6248#discussion_r1031769018
##########
flink/v1.16/flink/src/test/java/org/apache/iceberg/flink/source/TestFlinkTableSource.java:
##########
@@ -97,6 +97,7 @@ public void before() {
public void clean() {
sql("DROP TABLE IF EXISTS %s.%s", DATABASE_NAME, TABLE_NAME);
sql("DROP DATABASE IF EXISTS %s", DATABASE_NAME);
+ dropCatalog(CATALOG_NAME, true);
sql("DROP CATALOG IF EXISTS %s", CATALOG_NAME);
Review Comment:
Thanks for catching this
##########
flink/v1.16/flink/src/test/java/org/apache/iceberg/flink/FlinkTestBase.java:
##########
@@ -113,4 +113,15 @@ protected void assertSameElements(String message, Iterable<Row> expected, Iterab
.as(message)
.containsExactlyInAnyOrderElementsOf(expected);
}
+
+ /**
+ * We can not drop currently used catalog after FLINK-29677, so we have make sure that we do not use the current
+ * catalog before dropping it. This method switches to the 'default_catalog' and drops the one requested.
+ * @param catalogName The catalog to drop
+ * @param ifExist If we should use the 'IF EXISTS' when dropping the catalog
+ */
+ protected void dropCatalog(String catalogName, boolean ifExist) {
Review Comment:
Fixed
--
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] stevenzwu merged pull request #6248: Flink: Fix tests creating catalog after FLINK-29677
Posted by GitBox <gi...@apache.org>.
stevenzwu merged PR #6248:
URL: https://github.com/apache/iceberg/pull/6248
--
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