[#10464] refactor(iceberg): decouple listener events from Iceberg API models#10558
[#10464] refactor(iceberg): decouple listener events from Iceberg API models#10558
Conversation
… models Refactor Iceberg listener post/failure events to store serializable payload maps instead of Iceberg REST request/response classes. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR refactors Iceberg REST server listener post and failure event payloads to no longer expose Iceberg REST request/response model classes, using Map<String, Object> payloads instead to reduce cross-module coupling.
Changes:
- Replace event payload fields/getters from Iceberg REST request/response types to
Map<String, Object>across many table/namespace/view post & failure events. - Add
IcebergRESTUtils.toSerializableMap(Object)to convert (deep-clone) payload objects into JSON-serializable map structures. - Update event constructors to accept
Objectinputs and store them as serializable maps.
Reviewed changes
Copilot reviewed 22 out of 22 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/listener/api/event/IcebergUpdateTableFailureEvent.java | Update-table failure payload now stored as Map<String, Object>. |
| iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/listener/api/event/IcebergUpdateTableEvent.java | Update-table post event payloads now stored as maps. |
| iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/listener/api/event/IcebergUpdateNamespaceFailureEvent.java | Update-namespace failure payload now stored as a map. |
| iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/listener/api/event/IcebergUpdateNamespaceEvent.java | Update-namespace post event payloads now stored as maps. |
| iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/listener/api/event/IcebergReplaceViewFailureEvent.java | Replace-view failure payload now stored as a map. |
| iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/listener/api/event/IcebergReplaceViewEvent.java | Replace-view post event payloads now stored as maps. |
| iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/listener/api/event/IcebergRenameViewFailureEvent.java | Rename-view failure event signature changed to Object payload. |
| iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/listener/api/event/IcebergRenameViewEvent.java | Rename-view post event payload now stored as a map. |
| iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/listener/api/event/IcebergRenameTableFailureEvent.java | Rename-table failure event signature changed to Object payload. |
| iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/listener/api/event/IcebergRenameTableEvent.java | Rename-table post event payload now stored as a map. |
| iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/listener/api/event/IcebergRegisterTableFailureEvent.java | Register-table failure payload now stored as a map. |
| iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/listener/api/event/IcebergRegisterTableEvent.java | Register-table post event payloads now stored as maps. |
| iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/listener/api/event/IcebergLoadViewEvent.java | Load-view post payload now stored as a map. |
| iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/listener/api/event/IcebergLoadTableEvent.java | Load-table post payload now stored as a map. |
| iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/listener/api/event/IcebergLoadNamespaceEvent.java | Load-namespace post payload now stored as a map. |
| iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/listener/api/event/IcebergCreateViewFailureEvent.java | Create-view failure payload now stored as a map. |
| iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/listener/api/event/IcebergCreateViewEvent.java | Create-view post payloads now stored as maps. |
| iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/listener/api/event/IcebergCreateTableFailureEvent.java | Create-table failure payload now stored as a map. |
| iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/listener/api/event/IcebergCreateTableEvent.java | Create-table post payloads now stored as maps. |
| iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/listener/api/event/IcebergCreateNamespaceFailureEvent.java | Create-namespace failure payload now stored as a map. |
| iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/listener/api/event/IcebergCreateNamespaceEvent.java | Create-namespace post payloads now stored as maps. |
| iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/IcebergRESTUtils.java | Adds toSerializableMap(Object) helper used by event classes. |
| this.loadTableResponse = | ||
| IcebergRESTUtils.cloneIcebergRESTObject(loadTableResponse, LoadTableResponse.class); | ||
| this.updateTableRequest = IcebergRESTUtils.toSerializableMap(updateTableRequest); | ||
| this.loadTableResponse = IcebergRESTUtils.toSerializableMap(loadTableResponse); | ||
| } | ||
|
|
There was a problem hiding this comment.
The getter name createTableRequest() is inconsistent with the event and the underlying field (updateTableRequest). This looks like a copy/paste mistake and will confuse listener implementations. Consider adding a correctly named accessor (e.g., updateTableRequest()), and either renaming this method or deprecating it for backward compatibility.
| /** | |
| * Returns the serialized update table request associated with this event. | |
| * | |
| * @return A map representing the update table request. | |
| */ | |
| public Map<String, Object> updateTableRequest() { | |
| return updateTableRequest; | |
| } | |
| /** | |
| * @deprecated Use {@link #updateTableRequest()} instead. | |
| */ | |
| @Deprecated |
| public Map<String, Object> renameViewRequest() { | ||
| return replaceViewRequest; | ||
| } | ||
|
|
There was a problem hiding this comment.
The getter renameViewRequest() returns replaceViewRequest, which doesn’t match the operation (replace/alter view) and is misleading for API consumers. Consider exposing a replaceViewRequest() accessor (and optionally deprecating/renaming renameViewRequest() to preserve compatibility).
| public Map<String, Object> renameViewRequest() { | |
| return replaceViewRequest; | |
| } | |
| /** | |
| * Returns the serialized replace view request. | |
| * | |
| * @return the replace view request map | |
| */ | |
| public Map<String, Object> replaceViewRequest() { | |
| return replaceViewRequest; | |
| } | |
| /** | |
| * Returns the serialized replace view request. | |
| * | |
| * @return the replace view request map | |
| * @deprecated Use {@link #replaceViewRequest()} instead. | |
| */ | |
| @Deprecated | |
| public Map<String, Object> renameViewRequest() { | |
| return replaceViewRequest(); | |
| } |
| private final Object renameViewRequest; | ||
|
|
||
| public IcebergRenameViewFailureEvent( | ||
| IcebergRequestContext icebergRequestContext, | ||
| NameIdentifier viewIdentifier, | ||
| RenameTableRequest renameViewRequest, | ||
| Object renameViewRequest, | ||
| Exception e) { | ||
| super(icebergRequestContext, viewIdentifier, e); | ||
| this.renameViewRequest = renameViewRequest; | ||
| } | ||
|
|
||
| public RenameTableRequest renameViewRequest() { | ||
| public Object renameViewRequest() { | ||
| return renameViewRequest; | ||
| } |
There was a problem hiding this comment.
This failure event still stores and returns the raw request as Object, which is inconsistent with the PR’s stated goal of decoupling listener events from Iceberg REST models and making payloads serializable. Consider converting the request to a Map<String, Object> via IcebergRESTUtils.toSerializableMap(...) (matching the other post/failure events).
| private final Object renameTableRequest; | ||
|
|
||
| public IcebergRenameTableFailureEvent( | ||
| IcebergRequestContext icebergRequestContext, | ||
| NameIdentifier resourceIdentifier, | ||
| RenameTableRequest renameTableRequest, | ||
| Object renameTableRequest, | ||
| Exception e) { | ||
| super(icebergRequestContext, resourceIdentifier, e); | ||
| this.renameTableRequest = renameTableRequest; | ||
| } | ||
|
|
||
| public RenameTableRequest renameTableRequest() { | ||
| public Object renameTableRequest() { | ||
| return renameTableRequest; | ||
| } |
There was a problem hiding this comment.
This failure event still stores and returns the raw request as Object, which leaves listener payloads coupled to Iceberg REST request classes at runtime and may break serializability assumptions. Consider converting/storing it as Map<String, Object> via IcebergRESTUtils.toSerializableMap(...) for consistency with the other updated events.
| public static Map<String, Object> toSerializableMap(Object message) { | ||
| ObjectMapper icebergObjectMapper = IcebergObjectMapper.getInstance(); | ||
| try { | ||
| byte[] values = icebergObjectMapper.writeValueAsBytes(message); | ||
| return icebergObjectMapper.readValue(values, Map.class); | ||
| } catch (IOException e) { |
There was a problem hiding this comment.
toSerializableMap deserializes with Map.class and returns it as Map<String, Object>, which relies on an unchecked conversion and loses type information. Consider using a TypeReference<Map<String, Object>> (or JavaType) to keep the method type-safe and avoid warnings/possible key-type surprises.
| public static Map<String, Object> toSerializableMap(Object message) { | ||
| ObjectMapper icebergObjectMapper = IcebergObjectMapper.getInstance(); | ||
| try { | ||
| byte[] values = icebergObjectMapper.writeValueAsBytes(message); | ||
| return icebergObjectMapper.readValue(values, Map.class); |
There was a problem hiding this comment.
toSerializableMap is new behavior that changes listener payload shapes, but there’s no test coverage asserting that it actually produces a deep-cloned, serializable map (and not Iceberg model instances) for representative requests/responses. Consider adding a focused unit test (e.g., alongside the existing TestIcebergRESTUtils#testSerdeIcebergRESTObject) that validates the returned map structure and that mutations to the original object don’t affect the stored payload.
Create iceberg-listener-api module, move Iceberg listener events there, and remove direct Iceberg dependencies from the new module by using generic payload/event types with dispatcher-side serialization. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Code Coverage Report
Files
|
What changes were proposed in this pull request?
CreateTableRequest,LoadTableResponse) toMap<String, Object>payloads.IcebergRESTUtils.toSerializableMap(Object)to deep-clone event payloads into serializable map structures.Why are the changes needed?
Fix: #10464
Does this PR introduce any user-facing change?
Map<String, Object>instead of Iceberg-specific classes for affected post/failure events.How was this patch tested?
./gradlew :iceberg:iceberg-rest-server:compileJava./gradlew :iceberg:iceberg-rest-server:test --tests org.apache.gravitino.iceberg.service.rest.TestIcebergTableOperations --tests org.apache.gravitino.iceberg.service.rest.TestIcebergNamespaceOperations --tests org.apache.gravitino.iceberg.service.rest.TestIcebergViewOperations