apache
[FLINK-30481][FLIP-277] GlueCatalog Implementation
Purpose of the change
Add Glue catalog feature
Verifying this change
Tested in EMR cluster
- Unit test cases
Significant changes
(Please check any boxes [x] if the answer is "yes" )
- [x] Dependencies have been added or upgraded
- [x] Public API has been changed (Public API is any class annotated with
@Public(Evolving)) - [ ] Serializers have been changed
- [x] New feature has been introduced
- If yes, how is this documented? (not applicable / docs / JavaDocs / not documented)
- We need unit tests for everything
- Can we create e2e tests using localstack or something similar? I have not looked to see if anything exist
- Can we create e2e tests using localstack or something similar? I have not looked to see if anything exist
created separate jira to add e2e https://issues.apache.org/jira/browse/FLINK-30742 . adding e2e in same pr is getting large .
@dannycranmer please review whenever time .
- Added UT for the change
- Created seperate issue for adding e2e test for catalog and other subtasks to track seperately.
Had a discussion with @vahmed-hamdy .
- Added test for GlueCatalog
- @vahmed-hamdy helped with addressing to review comment https://github.com/apache/flink-connector-aws/pull/47#discussion_r1147446339
- for renametable implementation, doc update , e2e test for catalog , created seperate tasks
Please review in free time
@vahmed-hamdy , addressed to all your review comments, Please review the new changes whenever time.
Hi @Samrat002 Great Work! thanks alot, the PR is in a good state IMO.
I would add a test for the Factory class as well where we assert it creates catalog on happy case and test bunch of failure cases in validating the configs.
I left some minor comments, once addressed please loop in @dannycranmer to have a final review.
@dannycranmer , Please review in free time . I have made changes and increased the test coverage to 92% .
[gentle ping] @dannycranmer, i have addressed to all the review comments . Please review the PR whenever time
any updates here? This is a feature that is already available on Flink EMR would be great to have it here too.
vikramsinghchandel@ wckdman@ . thank you for your interest i will find a reviewer and close the feature
Thank you @foxus for the review . this PR was opened long ago. Let me try to address all the comments and test it again e2e . this PR will be ready for review by next 2 days
Thank you @foxus for the review .
this PR was opened long ago. Let me try to address all the comments and test it again e2e .
this PR will be ready for review by next 2 days
- [x] Many thanks, let me know when it's ready and I'll have another look. I noticed there were a couple of previous unaddressed comments from other reviewers - it will be great to get this one merged in.
Thank you @foxus for the review .
this PR was opened long ago. Let me try to address all the comments and test it again e2e .
this PR will be ready for review by next 2 days
Hey, let me know if I can help progress this? I'm happy to assist if you're willing and able to give me write access to the branch.
@foxus Apologies for the delay. i have mostly addressed to all the comments . Lastly wrapping up validating the working . i will ping you once the pr is ready for review
let me know if I can help progress this? I'm happy to assist if you're willing and able to give me write access to the branch.
yes . send invite for colaboration
@foxus Please review whenever time
Thanks for the effort responding to the comments, there's only one which I think has been marked as resolved but still appears not to have been addressed.
Fixed the missing comment. it might have slipped unintentionally
@foxus please review
@Samrat002, thanks for agreeing to collaborate, I've resolved some of the comments but left it as a separate commit to allow you to review and squash if you're happy whilst you're addressing the TypeMapper issue.
@foxus thank you for adding the change. 👍🏻 please review again i have added tests and tried to address to your suggested change.