Skip to content
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

[#6486] feat(iceberg): supports change Iceberg request in pre event listener #6485

Open
wants to merge 9 commits into
base: main
Choose a base branch
from

Conversation

FANNG1
Copy link
Contributor

@FANNG1 FANNG1 commented Feb 19, 2025

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
Copy link
Contributor Author

FANNG1 commented Feb 19, 2025

@jerryshao @puchengy PTAL.

@FANNG1 FANNG1 changed the title [SIP] supports change Iceberg create table request [#6486] feat(iceberg): supports change Iceberg create table request in event listener Feb 19, 2025
@puchengy
Copy link
Contributor

@FANNG1 LGTM, can we also support Update table as well to begin with? Thanks

@tengqm
Copy link
Contributor

tengqm commented Feb 20, 2025

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.

@FANNG1 FANNG1 changed the title [#6486] feat(iceberg): supports change Iceberg create table request in event listener [#6486] feat(iceberg): supports change Iceberg request in pre event listener Feb 20, 2025
@FANNG1
Copy link
Contributor Author

FANNG1 commented Feb 20, 2025

@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.

@tengqm
Copy link
Contributor

tengqm commented Feb 20, 2025

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.

return createTableRequestWrapper.get();
}

public ObjectWrapper<CreateTableRequest> createTableRequestWrapper() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is this function used for? I can't find the caller. Same questions for similar functions in the preEvent classes.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

event listener invokes this function to get ObjectWrapper and set an new CreateTableRequest

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@FANNG1 Is this an upcoming change or is this included in the PR? I searched around but do not see where this method is called.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There is no upcoming changes, it's the responsibility of end users to provide custom event listeners, like

  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);
    }
  }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Improvement] Allowing event listener change Iceberg create table request
3 participants