You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "ajantha-bhat (via GitHub)" <gi...@apache.org> on 2023/04/11 14:35:29 UTC
[GitHub] [iceberg] ajantha-bhat opened a new pull request, #7320: Nessie: Fix testcase failures
ajantha-bhat opened a new pull request, #7320:
URL: https://github.com/apache/iceberg/pull/7320
https://github.com/apache/iceberg/pull/6789 was merged recently and the PR didn't rebase before merging as there was no conflict.
But there was another PR that got merged https://github.com/apache/iceberg/pull/7146 which enforces the namespace creation. Hence the new tests failed.
Apologies for breaking the master build!
--
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] ajantha-bhat commented on pull request #7320: Nessie: Fix testcase failures
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on PR #7320:
URL: https://github.com/apache/iceberg/pull/7320#issuecomment-1503680976
PR is ready for merging.
--
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 #7320: Nessie: Fix testcase failures
Posted by "stevenzwu (via GitHub)" <gi...@apache.org>.
stevenzwu merged PR #7320:
URL: https://github.com/apache/iceberg/pull/7320
--
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] ajantha-bhat commented on pull request #7320: Nessie: Fix testcase failures
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on PR #7320:
URL: https://github.com/apache/iceberg/pull/7320#issuecomment-1503490157
cc: @Fokko, @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
[GitHub] [iceberg] nastra commented on a diff in pull request #7320: Nessie: Fix testcase failures
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7320:
URL: https://github.com/apache/iceberg/pull/7320#discussion_r1162948831
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestMultipleClients.java:
##########
@@ -98,12 +98,12 @@ public void testLoadNamespaceMetadata() {
@Test
public void testListTables() {
- catalog.createTable(TableIdentifier.parse("foo.tbl1"), schema);
+ createTable(catalog, TableIdentifier.parse("foo.tbl1"), schema);
Review Comment:
The concern I have is that creating the namespace for each catalog instance might actually hide a bug. That's why I think it's better to create the namespace once before the test starts
--
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 #7320: Nessie: Fix testcase failures
Posted by "stevenzwu (via GitHub)" <gi...@apache.org>.
stevenzwu commented on PR #7320:
URL: https://github.com/apache/iceberg/pull/7320#issuecomment-1503705964
merge this to fix the master branch build failure. thanks @ajantha-bhat for the fix and @nastra @Fokko @dimas-b for the review
--
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 #7320: Nessie: Fix testcase failures
Posted by "nastra (via GitHub)" <gi...@apache.org>.
nastra commented on code in PR #7320:
URL: https://github.com/apache/iceberg/pull/7320#discussion_r1162931296
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestMultipleClients.java:
##########
@@ -98,12 +98,12 @@ public void testLoadNamespaceMetadata() {
@Test
public void testListTables() {
- catalog.createTable(TableIdentifier.parse("foo.tbl1"), schema);
+ createTable(catalog, TableIdentifier.parse("foo.tbl1"), schema);
Review Comment:
why not just call `catalog.createNamespace(Namespace.of("foo"), Collections.emptyMap());` at the beginning of these methods?
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestMultipleClients.java:
##########
@@ -98,12 +98,12 @@ public void testLoadNamespaceMetadata() {
@Test
public void testListTables() {
- catalog.createTable(TableIdentifier.parse("foo.tbl1"), schema);
+ createTable(catalog, TableIdentifier.parse("foo.tbl1"), schema);
Assertions.assertThat(catalog.listTables(Namespace.of("foo")))
.containsExactlyInAnyOrder(TableIdentifier.parse("foo.tbl1"));
// another client creates a table with the same nessie server
- anotherCatalog.createTable(TableIdentifier.parse("foo.tbl2"), schema);
+ createTable(anotherCatalog, TableIdentifier.parse("foo.tbl2"), schema);
Review Comment:
I don't think it's necessary to potentially create the namespace again here. Just calling `catalog.createNamespace(Namespace.of("foo"), Collections.emptyMap());` at the beginning of the method should be enough.
Same applies for the other test methods in this class that fail.
--
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] ajantha-bhat commented on a diff in pull request #7320: Nessie: Fix testcase failures
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #7320:
URL: https://github.com/apache/iceberg/pull/7320#discussion_r1162943940
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestMultipleClients.java:
##########
@@ -98,12 +98,12 @@ public void testLoadNamespaceMetadata() {
@Test
public void testListTables() {
- catalog.createTable(TableIdentifier.parse("foo.tbl1"), schema);
+ createTable(catalog, TableIdentifier.parse("foo.tbl1"), schema);
Review Comment:
Suggestion will work but I am following the style from other testcases in Nessie introduced in #7146
--
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] ajantha-bhat commented on a diff in pull request #7320: Nessie: Fix testcase failures
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #7320:
URL: https://github.com/apache/iceberg/pull/7320#discussion_r1162954525
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestMultipleClients.java:
##########
@@ -98,12 +98,12 @@ public void testLoadNamespaceMetadata() {
@Test
public void testListTables() {
- catalog.createTable(TableIdentifier.parse("foo.tbl1"), schema);
+ createTable(catalog, TableIdentifier.parse("foo.tbl1"), schema);
Assertions.assertThat(catalog.listTables(Namespace.of("foo")))
.containsExactlyInAnyOrder(TableIdentifier.parse("foo.tbl1"));
// another client creates a table with the same nessie server
- anotherCatalog.createTable(TableIdentifier.parse("foo.tbl2"), schema);
+ createTable(anotherCatalog, TableIdentifier.parse("foo.tbl2"), schema);
Review Comment:
yeah, creating again from a different client is not required as namespace already exist. I removed it.
--
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] ajantha-bhat commented on a diff in pull request #7320: Nessie: Fix testcase failures
Posted by "ajantha-bhat (via GitHub)" <gi...@apache.org>.
ajantha-bhat commented on code in PR #7320:
URL: https://github.com/apache/iceberg/pull/7320#discussion_r1162957254
##########
nessie/src/test/java/org/apache/iceberg/nessie/TestMultipleClients.java:
##########
@@ -98,12 +98,12 @@ public void testLoadNamespaceMetadata() {
@Test
public void testListTables() {
- catalog.createTable(TableIdentifier.parse("foo.tbl1"), schema);
+ createTable(catalog, TableIdentifier.parse("foo.tbl1"), schema);
Review Comment:
Fixed that creating namespace from another catalog part and still following the same style of other testcases. If still required to manually create a namespace, I can do that. But I don't think necessary.
--
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] Fokko commented on pull request #7320: Nessie: Fix testcase failures
Posted by "Fokko (via GitHub)" <gi...@apache.org>.
Fokko commented on PR #7320:
URL: https://github.com/apache/iceberg/pull/7320#issuecomment-1503705299
Thanks @stevenzwu for merging :)
--
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