Skip to content

Add Arguments to Various Event Records #2765

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.

Sign up for GitHub

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

Merged
merged 1 commit into from
Oct 8, 2025
Merged

Add Arguments to Various Event Records #2765

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
29 changes: 23 additions & 6 deletions ...a/org/apache/polaris/service/catalog/iceberg/IcebergRestCatalogEventServiceDelegator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -275,7 +275,8 @@ public Response loadTable(
realmContext,
securityContext);
polarisEventListener.onAfterLoadTable(
new AfterLoadTableEvent(catalogName, namespaceObj, (LoadTableResponse) resp.getEntity()));
new AfterLoadTableEvent(
catalogName, namespaceObj, table, (LoadTableResponse) resp.getEntity()));
Comment on lines -278 to +279
Copy link
Contributor

@eric-maynard eric-maynard Oct 6, 2025

Choose a reason for hiding this comment

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

In general I feel this highlights a problem with the newly-refactored events API -- there seems to be an assumption that the event content will always be accessible through the response, which really may not be the case.

Indeed, if all the event's information can be derived from the request/response pairs, why not just pass the request/response pairs in to the listener? Why have event types at all?

Copy link
Contributor Author

@adnanhemani adnanhemani Oct 7, 2025

Choose a reason for hiding this comment

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

Request POJOs don't always have all the fields unfortunately either. I get the point though and agree with it generally.

My thought process has been to ensure that all information regarding what was queried or changed as part of the request should be available in one way or another in the After event using the following guidelines:

  • If the information is regarding a Polaris entity, then the entity's hierarchy should be in distinct fields within the event.
  • If the request is for a modification of sorts, then the response object should be placed as-is in the After event.
  • All other relevant objects should be placed as similarly to the request object into the event.

I know this isn't being followed across the board - but does this make sense? Happy to make changes where this isn't the case or look into a refactor if you believe in different guidelines.

return resp;
}

Expand Down Expand Up @@ -331,7 +332,10 @@ public Response registerTable(
prefix, namespace, registerTableRequest, realmContext, securityContext);
polarisEventListener.onAfterRegisterTable(
new AfterRegisterTableEvent(
catalogName, namespaceObj, (LoadTableResponse) resp.getEntity()));
catalogName,
namespaceObj,
registerTableRequest.name(),
(LoadTableResponse) resp.getEntity()));
return resp;
}

Expand Down Expand Up @@ -367,7 +371,11 @@ public Response updateTable(
prefix, namespace, table, commitTableRequest, realmContext, securityContext);
polarisEventListener.onAfterUpdateTable(
new AfterUpdateTableEvent(
catalogName, namespaceObj, table, (LoadTableResponse) resp.getEntity()));
catalogName,
namespaceObj,
table,
commitTableRequest,
(LoadTableResponse) resp.getEntity()));
return resp;
}

Expand All @@ -385,7 +393,11 @@ public Response createView(
Response resp =
delegate.createView(prefix, namespace, createViewRequest, realmContext, securityContext);
polarisEventListener.onAfterCreateView(
new AfterCreateViewEvent(catalogName, namespaceObj, (LoadViewResponse) resp.getEntity()));
new AfterCreateViewEvent(
catalogName,
namespaceObj,
createViewRequest.name(),
(LoadViewResponse) resp.getEntity()));
return resp;
}

Expand Down Expand Up @@ -436,7 +448,8 @@ public Response loadView(
polarisEventListener.onBeforeLoadView(new BeforeLoadViewEvent(catalogName, namespaceObj, view));
Response resp = delegate.loadView(prefix, namespace, view, realmContext, securityContext);
polarisEventListener.onAfterLoadView(
new AfterLoadViewEvent(catalogName, namespaceObj, (LoadViewResponse) resp.getEntity()));
new AfterLoadViewEvent(
catalogName, namespaceObj, view, (LoadViewResponse) resp.getEntity()));
return resp;
}

Expand Down Expand Up @@ -504,7 +517,11 @@ public Response replaceView(
prefix, namespace, view, commitViewRequest, realmContext, securityContext);
polarisEventListener.onAfterReplaceView(
new AfterReplaceViewEvent(
catalogName, namespaceObj, view, (LoadViewResponse) resp.getEntity()));
catalogName,
namespaceObj,
view,
commitViewRequest,
(LoadViewResponse) resp.getEntity()));
return resp;
}

Expand Down
21 changes: 16 additions & 5 deletions ...ime/service/src/main/java/org/apache/polaris/service/events/IcebergRestCatalogEvents.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -121,7 +121,10 @@ public record BeforeLoadTableEvent(
implements PolarisEvent {}

public record AfterLoadTableEvent(
String catalogName, Namespace namespace, LoadTableResponse loadTableResponse)
String catalogName,
Namespace namespace,
String tableName,
LoadTableResponse loadTableResponse)
implements PolarisEvent {}

public record BeforeCheckExistsTableEvent(String catalogName, Namespace namespace, String table)
Expand All @@ -143,7 +146,10 @@ public record BeforeRegisterTableEvent(
implements PolarisEvent {}

public record AfterRegisterTableEvent(
String catalogName, Namespace namespace, LoadTableResponse loadTableResponse)
String catalogName,
Namespace namespace,
String tableName,
LoadTableResponse loadTableResponse)
implements PolarisEvent {}

public record BeforeRenameTableEvent(String catalogName, RenameTableRequest renameTableRequest)
Expand All @@ -163,6 +169,7 @@ public record AfterUpdateTableEvent(
String catalogName,
Namespace namespace,
String sourceTable,
CommitTableRequest commitTableRequest,
LoadTableResponse loadTableResponse)
implements PolarisEvent {}

Expand All @@ -172,7 +179,7 @@ public record BeforeCreateViewEvent(
implements PolarisEvent {}

public record AfterCreateViewEvent(
String catalogName, Namespace namespace, LoadViewResponse loadViewResponse)
String catalogName, Namespace namespace, String viewName, LoadViewResponse loadViewResponse)
implements PolarisEvent {}

public record BeforeListViewsEvent(String catalogName, Namespace namespace)
Expand All @@ -185,7 +192,7 @@ public record BeforeLoadViewEvent(String catalogName, Namespace namespace, Strin
implements PolarisEvent {}

public record AfterLoadViewEvent(
String catalogName, Namespace namespace, LoadViewResponse loadViewResponse)
String catalogName, Namespace namespace, String viewName, LoadViewResponse loadViewResponse)
implements PolarisEvent {}

public record BeforeCheckExistsViewEvent(String catalogName, Namespace namespace, String view)
Expand Down Expand Up @@ -214,7 +221,11 @@ public record BeforeReplaceViewEvent(
implements PolarisEvent {}

public record AfterReplaceViewEvent(
String catalogName, Namespace namespace, String sourceView, LoadViewResponse loadViewResponse)
String catalogName,
Namespace namespace,
String sourceView,
CommitViewRequest commitViewRequest,
LoadViewResponse loadViewResponse)
implements PolarisEvent {}

// Credential Events
Expand Down