-
Notifications
You must be signed in to change notification settings - Fork 319
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
[#5370] feat(iceberg): make IcebergTableOperationDispatcher interface more extendable #5369
base: main
Are you sure you want to change the base?
Conversation
Do we plan to make similar changes to |
new IcebergCreateTablePreEvent( | ||
PrincipalUtils.getCurrentUserName(), nameIdentifier, createTableRequest)); |
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.
Would it be possible to pass the whole context object inside pre-listener and post listener?
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.
Could you describe the scenarios?
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.
Yes, the use case is logging request headers within the event listener.
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.
Sounds reasonable, @jerryshao 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.
My feeling is that exposing the request header to the event seems not so good, makes the event listener too flexible to do anything. If we want to do check request header check beforehand, maybe we can implement some filters in Jetty, and let Gravitino have the ability to load the custom filters.
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.
@jerryshao Hi Jerry, I think your idea works for filter checking. What about the use case of headers logging along with table events? How would that look like? Thanks.
yes, will do |
What changes were proposed in this pull request?
add
IcebergRequestContext
inIcebergTableOperationDispatcher
Why are the changes needed?
Fix: #5370
Does this PR introduce any user-facing change?
No
How was this patch tested?
existing tests