apache
[#6486] feat(iceberg): supports change Iceberg request in pre event listener
What changes were proposed in this pull request?
Use an wrapper class to allow user create an new request, and the dispatcher operator use the new created request. The event listener could leverage ObjectWrapper to create new Iceberg request.
public void onPreEvent(PreEvent preEvent) {
if (preEvent instanceof IcebergCreateTablePreEvent) {
ObjectWrapper<CreateTableRequest> objectWrapper = ((IcebergCreateTablePreEvent) preEvent).createTableRequestWrapper();
CreateTableRequest originalRequest = objectWrapper.get();
// create newRequest according to your business logic
objectWrapper.set(newRequest);
}
}
Why are the changes needed?
Fix: #6486
Does this PR introduce any user-facing change?
no
How was this patch tested?
change the iceberg request and it take affects when creating table in local enviroment
@FANNG1 LGTM, can we also support Update table as well to begin with? Thanks
I have a concern with this piece solution. Either we need to augment the create table request to meet the requirement, or we need a global solution for events. By global solution, I mean we may need a wrapper pattern for all events. Of course we can get his one case handled today, but in the long run, we have to make sure the same thing won't happen again.
@puchengy @tengqm thanks for your advise, use a more general solution by objectWrapper to wrap the Iceberg REST request in pre event, please help to review again.
Regarding this PR, I think it is clean and okay to go ahead. Wondering if we need an umbrella issue for handling other events so that we won't get the ball dropped.
On a quick review, my feeling is that using a wrapper is not so intuitive, do we have any better solution to handle such scenario?
On a quick review, my feeling is that using a wrapper is not so intuitive, do we have any better solution to handle such scenario?
One thing I am thinking is if we can create a MutableRequestBuilder that wraps the exiting Request, the existing Iceberg request builder does not support get method (maybe something good to contribute) otherwise below is my proposal:
class MutableCreateTableRequestBuilder {
// clone existing unmodifiable CreateTableRequest into a modifiable builder
MutableCreateTableRequestBuilder(CreateTableRequest request) {
this.name = request.name()
this.properties = request.properties()
...
}
void setProperties(Map properties) {}
Map getProperties() {}
void setLocation(String newLocation) {}
String getLocation() {}
...
CreateTableRequest build() {
CreateTableRequest.Builder builder = ...
builder.set...
builcer.set...
return builder.build()
}
}
As a result MutableCreateTableRequestBuilder will be the element that is passed around and IRC will be responsible to call build() function
@FANNG1 thought?
On a quick review, my feeling is that using a wrapper is not so intuitive, do we have any better solution to handle such scenario?
One thing I am thinking is if we can create a MutableRequestBuilder that wraps the exiting Request, the existing Iceberg request builder does not support get method (maybe something good to contribute) otherwise below is my proposal:
class MutableCreateTableRequestBuilder { // clone existing unmodifiable CreateTableRequest into a modifiable builder MutableCreateTableRequestBuilder(CreateTableRequest request) { this.name = request.name() this.properties = request.properties() ... } void setProperties(Map properties) {} Map getProperties() {} void setLocation(String newLocation) {} String getLocation() {} ... CreateTableRequest build() { CreateTableRequest.Builder builder = ... builder.set... builcer.set... return builder.build() } }As a result
MutableCreateTableRequestBuilderwill be the element that is passed around and IRC will be responsible to call build() function@FANNG1 thought?
I'm not sure whether MutableCreateTableRequestBuilder is suitable to place in Gravitino Iceberg REST server code, as it's tied to Iceberg request implemetation.
On a quick review, my feeling is that using a wrapper is not so intuitive, do we have any better solution to handle such scenario?
Let me take more time to think about it
How about defining a transform interface like T -> T for pre-event? If we have to introduce this layer, we'd better make it more clear for the users, The concept of wrapper seems too hacky.
@jerryshao @puchengy , I add a new method in EventListenerPlugin to transform the pre-event, is it the right direction? If yes, I'll do more work to polish the implementation. thx.
/**
* Transform a preEvent before it is processed by the listener.
*
* <p>This method allows the plugin to modify the preEvent before it is processed. The sequence
* of the transformations is determined by the event listener configuration order.
*
* @param preEvent The preEvent to be transformed.
* @return The transformed preEvent.
*/
default PreEvent transformPreEvent(PreEvent preEvent) {
return preEvent;
};
@FANNG1 Hi thanks for working on this.
The sequence of the transformations is determined by the event listener configuration order.
Can you share a little more on how it will look like? Thanks
@FANNG1 Hi thanks for working on this.
The sequence of the transformations is determined by the event listener configuration order.
Can you share a little more on how it will look like? Thanks
We may add multiple event listeners like gravitino.eventListener.names = listener1, listener2, the transform logic in listener1 is executed first. But after another thought I prefer doesn't give such guarantee for transform order, because the transform logic should not have dependences, we should grant the transform logic is invoked before process logic in onPreEvent. WDYT?
@FANNG1 SGTM, I don't have much preference right, maybe I will be able to comment on once you have a PR
Adding SupportsChangingPreEvent interface to declare whether a pre-event supports changing the content to avoid the scenario that the event listener changes the content of the pre-event, but it doesn't take effect.
I'm generally OK with this solution, but I think the PR is not ready, right? We shall support SupportsChangingPreEvent for preevent, right?
I'm generally OK with this solution, but I think the PR is not ready, right? We shall support
SupportsChangingPreEventfor preevent, right?
Yes, this is the draft stage to show the solutions. I'll continue the work if no opposing comments. cc @puchengy
seems no requirement to changing load/list/delete/exists requests, so only make create/update pre-event could be modified, though implementing SupportsChangingPreEvent. @jerryshao @puchengy PTAL.
@FANNG1 Thanks for the work. I will spend sometime tomorrow to look at the PR!