Add Events for PolarisServiceImpl APIs

[adnanhemani]

Adding instrumentation for events for all events defined in PolarisServiceImpl.java. Not all events will be useful and/or necessary - but allow users to plug in with custom event listeners for any of these APIs. The default event listener is a no-op.

The main file to review here is PolarisServiceImpl. All new events are simply just a constructor and getter methods.

[eric-maynard]

LGTM! This isn't all the new events we'll need to add but it's a good start.

[adnanhemani]

@eric-maynard - any way to retrigger the tests? Seems like it failed on a processing step unrelated to the changes.

[snazy]

This PR received a lot of comments, should wait for @adutra's review as well.

[snazy]

This change exposes secrets to event listeners, which is quite concerning.

I've posted a bunch of comments for discussion about the approach on the dev-ML.

[eric-maynard]

Still LGTM, and the secrets are gone from the events now which is good

[eric-maynard]

@snazy are there still changes you'd like to see here?

[snazy]

Yea, I think we should simplify and enhance the API, as proposed on dev@

[adnanhemani]

@snazy - I've replied on the referenced email thread >2 weeks ago but there is not any response to any of the comments on the thread since then. If you have a hard opinion on this, can you please reply to the email thread to keep this work moving forward?

If there is not a hard opinion, I will continue pushing to get this PR merged as-is.

[snazy]

My impression was that you're working on the proposed ideas.

My (hard) opinion on this is that we have to allow event listeners to
a) associate after with before events
b) opt in to get success/failure results
and to reduce the amount of code that is necessary in call sites and listener implementations and events themselves.

[singhpk234]

can we have all the records in one class ?

[eric-maynard]

The current events aren't all in one file, so while I think grouping them that way is fine we should be consistent.

IIUC association* of different events is possible (and intended) on the EventListener side.

[adnanhemani]

The current events aren't all in one file, so while I think grouping them that way is fine we should be consistent.

Agreed, I can do this in the next round of revisions. But am waiting for the followup meeting regarding events to put further work in on this PR.

IIUC associated of different events is possible (and intended) on the EventListener side.

This is correct as well. @snazy - does that resolve your concerns?

[snazy]

Thanks for removing the secrets.

But I still believe that it can be simplified, and have the currently missing the failure notifications. I've proposed a way to achieve this using Java pseudo code on the dev-ML. Adding it here as well below.

public interface EventListener {
  /**
   * "Dummy event foo" about to happen, with parameters x + y, no additional information collected
   * for "after the fact".
   */
  InFlightVoid beforeEventFoo(String x, int y);

  /** About to commit to an Iceberg table. */
  InFlight<IcebergTableCommitted> beforeIcebergTableCommit(UpdateTableRequest updateTableRequest);
}

public interface InFlight<SUCCESS_PAYLOAD> {
  /** Called "after the fact". */
  void success(SUCCESS_PAYLOAD payload);

  /** Potentially called "after the fact" in case of a failure. */
  void failure(Throwable throwable);

  /** Potentially called "after the fact" in case of a failure. */
  void failure(String reason);
}

/** Convenience for events that do not have a success-payload. */
public interface InFlightVoid extends InFlight<Void> {
  default void success() {
    success(null);
  }
}

/** Success-payload for table-committed. */
public record IcebergTableCommitted(TableMetadata originalState, TableMetadata newState) {}

Example for a simple event "foo", w/o a success-payload:

public interface Foo {
  void doFoo();
}

class FooImpl implements Foo {
  EventListener eventListener;

  @Override
  public void doFoo() {
    InFlightVoid event = eventListener.beforeEventFoo("foo", 42);
    try {
      // do foo stuff
      event.success();
    } catch (Exception x) {
      event.failure(x);
    }
  }
}

Example for table committed, with update-request, before and after states. Up to the event listener implementation which parts are consumed.

public interface Commit {
  void doCommit(UpdateTableRequest updateTableRequest);
}

class CommitImpl implements Commit {
  EventListener eventListener;

  @Override
  public void doCommit(UpdateTableRequest updateTableRequest) {
    InFlight<IcebergTableCommitted> event =
        eventListener.beforeIcebergTableCommit(updateTableRequest);
    try {
      // do table commit
      TableMetadata previousTableMetadata = ...;
      if (somePreconditionFailed) {
        event.failure("Precondition xyz failed");
        return;
      }
      TableMetadata updatedTableMetadata = ...;
      event.success(new IcebergTableCommitted(previousTableMetadata, updatedTableMetadata));
    } catch (Exception x) {
      event.failure(x);
    }
  }
}

[adnanhemani]

Further discussion on stateful event handling is on the Dev ML.

[adnanhemani]

Closed in favor of #2482.