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/09/19 15:59:45 UTC
[GitHub] [iceberg] JonasJ-ap opened a new pull request, #5797: AWS: Fix issue that GlueTableOperations fails to initialize FileIO when GlueCatalog is not initialized with catalogProperties
JonasJ-ap opened a new pull request, #5797:
URL: https://github.com/apache/iceberg/pull/5797
When the user initialize `GlueCatalog` without providing a `catalogProperties`, the corresponding `GlueTableOperations` will face a `NullPointerException` when initializing FileIO using `initializeFileIO`. This bug causes several tests in AWS Integration test fails.
## before this fix:
## after this fix:
The remaining test errors are either due to the test itself or outdated environment settings. Since they are not related to `GlueCatalog`, they will be addressed in another PR: #5784
--
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] JonasJ-ap commented on a diff in pull request #5797: AWS: Fix issue that GlueTableOperations fails to initialize FileIO when GlueCatalog is not initialized with catalogProperties
Posted by GitBox <gi...@apache.org>.
JonasJ-ap commented on code in PR #5797:
URL: https://github.com/apache/iceberg/pull/5797#discussion_r974721236
##########
aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java:
##########
@@ -228,13 +228,7 @@ protected TableOperations newTableOps(TableIdentifier tableIdentifier) {
}
return new GlueTableOperations(
- glue,
- lockManager,
- catalogName,
- awsProperties,
- catalogProperties,
Review Comment:
The `catalogProperties` here is always null. Hence, I replaced it with a `properties()` to initialize an empty ImmutableMap to avoid any `NullPointerException`
--
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] JonasJ-ap commented on pull request #5797: AWS: Fix issue that GlueTableOperations fails to initialize FileIO when GlueCatalog is not initialized with catalogProperties
Posted by GitBox <gi...@apache.org>.
JonasJ-ap commented on PR #5797:
URL: https://github.com/apache/iceberg/pull/5797#issuecomment-1251798384
@danielcweeks, @danielcweeks. Thank you for your suggestions. In this case, I will close this PR and open another one regarding the code simplification after the correction in #5784.
--
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] JonasJ-ap closed pull request #5797: AWS: Fix issue that GlueTableOperations fails to initialize FileIO when GlueCatalog is not initialized with catalogProperties
Posted by GitBox <gi...@apache.org>.
JonasJ-ap closed pull request #5797: AWS: Fix issue that GlueTableOperations fails to initialize FileIO when GlueCatalog is not initialized with catalogProperties
URL: https://github.com/apache/iceberg/pull/5797
--
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] amogh-jahagirdar commented on a diff in pull request #5797: AWS: Fix issue that GlueTableOperations fails to initialize FileIO when GlueCatalog is not initialized with catalogProperties
Posted by GitBox <gi...@apache.org>.
amogh-jahagirdar commented on code in PR #5797:
URL: https://github.com/apache/iceberg/pull/5797#discussion_r974848173
##########
aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java:
##########
@@ -228,13 +228,7 @@ protected TableOperations newTableOps(TableIdentifier tableIdentifier) {
}
return new GlueTableOperations(
- glue,
- lockManager,
- catalogName,
- awsProperties,
- catalogProperties,
Review Comment:
Yeah the expectation is that for loading the catalog dynamically, all the fields (including catalogProperties) are set via initialize() as @danielcweeks suggested, so I think let's just fix the integration test catalog initialization.
As for simplifying the code, sure I think we can have a common return point through properties(); I would say let's correct the integration tests in https://github.com/apache/iceberg/pull/5784 and then we can come back here, and re-run the tests after the common return point change. If they succeed, then we should be good.
--
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] JonasJ-ap commented on pull request #5797: AWS: Fix issue that GlueTableOperations fails to initialize FileIO when GlueCatalog is not initialized with catalogProperties
Posted by GitBox <gi...@apache.org>.
JonasJ-ap commented on PR #5797:
URL: https://github.com/apache/iceberg/pull/5797#issuecomment-1251237548
@amogh-jahagirdar Would you mind reviewing this fix and giving some suggestions if needed? Thank you in advance for your help
--
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] JonasJ-ap commented on a diff in pull request #5797: AWS: Fix issue that GlueTableOperations fails to initialize FileIO when GlueCatalog is not initialized with catalogProperties
Posted by GitBox <gi...@apache.org>.
JonasJ-ap commented on code in PR #5797:
URL: https://github.com/apache/iceberg/pull/5797#discussion_r974721236
##########
aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java:
##########
@@ -228,13 +228,7 @@ protected TableOperations newTableOps(TableIdentifier tableIdentifier) {
}
return new GlueTableOperations(
- glue,
- lockManager,
- catalogName,
- awsProperties,
- catalogProperties,
Review Comment:
The `catalogProperties` here is always null. Hence, I used `properties()` to initialize an empty ImmutableMap to avoid any `NullPointerException`
--
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 #5797: AWS: Fix issue that GlueTableOperations fails to initialize FileIO when GlueCatalog is not initialized with catalogProperties
Posted by GitBox <gi...@apache.org>.
danielcweeks commented on code in PR #5797:
URL: https://github.com/apache/iceberg/pull/5797#discussion_r974777890
##########
aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java:
##########
@@ -228,13 +228,7 @@ protected TableOperations newTableOps(TableIdentifier tableIdentifier) {
}
return new GlueTableOperations(
- glue,
- lockManager,
- catalogName,
- awsProperties,
- catalogProperties,
Review Comment:
Hey @JonasJ-ap, I think this means that the GlueCatalog isn't being correctly initialized in the integration tests. The `initialize()` call should set the value correctly. I'm guessing some of the testing init methods aren't consistent with the implementation in from the catalog interface.
--
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] JonasJ-ap commented on a diff in pull request #5797: AWS: Fix issue that GlueTableOperations fails to initialize FileIO when GlueCatalog is not initialized with catalogProperties
Posted by GitBox <gi...@apache.org>.
JonasJ-ap commented on code in PR #5797:
URL: https://github.com/apache/iceberg/pull/5797#discussion_r974836109
##########
aws/src/main/java/org/apache/iceberg/aws/glue/GlueCatalog.java:
##########
@@ -228,13 +228,7 @@ protected TableOperations newTableOps(TableIdentifier tableIdentifier) {
}
return new GlueTableOperations(
- glue,
- lockManager,
- catalogName,
- awsProperties,
- catalogProperties,
Review Comment:
Thank you so much for your review and suggestions. Now I see that there are some integration tests using the wrong `initialize()` call and thus skip initializing this particular field. I will update the changes to those tests in #5784.
Meanwhile, I am curious if the return statement here is worth a change. Given that this return statement always receives null for `catalogProperties` and methods in `GlueTableOperations` do not fully handle the `null` case, it seems not a good idea to initialize `GlueTableOperations` in this way. Maybe we could still call `properties()` here or delete this return statement since it is unreachable under the current code logic? I appreciate your time and help.
--
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