-
Notifications
You must be signed in to change notification settings - Fork 384
(NOT_READY) Fixed IcebergCatalog stale FileIO after metadata refresh #3494
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Closed
Closed
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why do we request R/W access for FileIO in a "refresh" method?
Having to reset
tableFileIOin this context sounds like a design issue to me. I'd think access level (Read / Write / List) should be decided per use case (load vs. commit, etc.).WDYT?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, resetting the permissions here does feel a bit odd. So we may need to do some refactor to fix this bug. Based on my understanding, the issue reporter faced was AWS assumeRole credentials became stale after table creation due to cached FileIO thus a quick "workaround" here is to reset it to full which then allowed the reporter to continue the test and validated the root cause. If we do want to keep the FileIO cached, we may need to do reader/writer FileIO separately to follow the least privileges pattern. Another alternative would be creating new FileIO per operation and right permissions for them. Any preferred route or better solutions?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I personally need to think more about this. I'm not totally familiar with this code.
If someone else is available for a review - please comment.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fabio-rizzo-01 do you mind take a look?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@dennishuo : WDYT?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm leaning towards option 2. Do we know how often this FileIO instance is actually resued in runtime?
Sorry, I'm not very familiar with this area of the code myself, but I imagine every new REST request gets a new FileIO anyway, so reuse is limited to one request, I guess 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think so. I am also a bit new to this specific code path but the cached FileIO appears to be the problem in this case. Let me update to use option 2 and avoid cache.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I tried to implement the fix but it ends up being a major refactor and we may needed a delegate FileIO class.
Based on my understanding, the issue here is for that specific setup (I also tried to see if I can get something similar setup on my local for validation but no luck so far as setup details are not being shared nor a minimal reproducible), after spark use its RO credential with assume role privilege to assume the role associated with catalog, it won't be able to use the STS token for insert thus falled back to client id/secret associated with spark client. Which in this case, it can only happened after the metadata refresh (as it will need to be call first before insert can happen based on my understanding). Then spark will use the returned FileIO from function
io()which can lost write access (thus the initial fix worked).However, for the setup I have on AWS, the client role always doesn't have write access to the bucket directly and it uses assume role with the IAM role associated with the catalog for all operations. So I believed this may be very setup specific for this issue.
I am bit hesitated to do the major refactor for delegate FileIO (so permission can be determined based on context such as RO for
io().newInputFile()and RW forio().newOutputFile()) as I don't have a setup which I can test tp validate as well as future refactors which can break this silently as well without some proper test cases/setups.Maybe we should have someone who is more familiar with this code path to take another look in case I missed something here. cc @dimas-b @evindj @flyrain
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@MonkeyCanCode : Thanks for the investigation!
From my side, I'm willing to look deeper into refactoring this code, but I do not really understand the problem (yet) 😅
Do you have repro instructions handy? Could you post them here or summarize in #3440 ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Unfortunately, we don't have a reproducible thus the hesitation for major refactor. Detailed added in #3440. Thanks for looking this this. I will set this as not ready for review.