Copilot commented on code in PR #10558:
URL: https://github.com/apache/gravitino/pull/10558#discussion_r2994511541
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/listener/api/event/IcebergRenameTableFailureEvent.java:
##########
@@ -21,23 +21,22 @@
import org.apache.gravitino.NameIdentifier;
import org.apache.gravitino.annotation.DeveloperApi;
-import org.apache.iceberg.rest.requests.RenameTableRequest;
/** Represent an event when rename Iceberg table failed. */
@DeveloperApi
public class IcebergRenameTableFailureEvent extends IcebergTableFailureEvent {
- private final RenameTableRequest renameTableRequest;
+ 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;
}
Review 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.
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/listener/api/event/IcebergUpdateTableEvent.java:
##########
@@ -19,36 +19,33 @@
package org.apache.gravitino.listener.api.event;
+import java.util.Map;
import org.apache.gravitino.NameIdentifier;
import org.apache.gravitino.annotation.DeveloperApi;
import org.apache.gravitino.iceberg.service.IcebergRESTUtils;
-import org.apache.iceberg.rest.requests.UpdateTableRequest;
-import org.apache.iceberg.rest.responses.LoadTableResponse;
/** Represent an event after updating Iceberg table successfully. */
@DeveloperApi
public class IcebergUpdateTableEvent extends IcebergTableEvent {
- private final UpdateTableRequest updateTableRequest;
- private final LoadTableResponse loadTableResponse;
+ private final Map<String, Object> updateTableRequest;
+ private final Map<String, Object> loadTableResponse;
public IcebergUpdateTableEvent(
IcebergRequestContext icebergRequestContext,
NameIdentifier resourceIdentifier,
- UpdateTableRequest updateTableRequest,
- LoadTableResponse loadTableResponse) {
+ Object updateTableRequest,
+ Object loadTableResponse) {
super(icebergRequestContext, resourceIdentifier);
- this.updateTableRequest =
- IcebergRESTUtils.cloneIcebergRESTObject(updateTableRequest,
UpdateTableRequest.class);
- this.loadTableResponse =
- IcebergRESTUtils.cloneIcebergRESTObject(loadTableResponse,
LoadTableResponse.class);
+ this.updateTableRequest =
IcebergRESTUtils.toSerializableMap(updateTableRequest);
+ this.loadTableResponse =
IcebergRESTUtils.toSerializableMap(loadTableResponse);
}
Review 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.
```suggestion
/**
* 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
```
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/IcebergRESTUtils.java:
##########
@@ -115,6 +115,16 @@ public static <T> T cloneIcebergRESTObject(Object message,
Class<T> className) {
}
}
+ public static Map<String, Object> toSerializableMap(Object message) {
+ ObjectMapper icebergObjectMapper = IcebergObjectMapper.getInstance();
+ try {
+ byte[] values = icebergObjectMapper.writeValueAsBytes(message);
+ return icebergObjectMapper.readValue(values, Map.class);
Review 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.
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/listener/api/event/IcebergRenameViewFailureEvent.java:
##########
@@ -21,23 +21,22 @@
import org.apache.gravitino.NameIdentifier;
import org.apache.gravitino.annotation.DeveloperApi;
-import org.apache.iceberg.rest.requests.RenameTableRequest;
/** Represent an event when rename Iceberg view failed. */
@DeveloperApi
public class IcebergRenameViewFailureEvent extends IcebergViewFailureEvent {
- private final RenameTableRequest renameViewRequest;
+ 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;
}
Review 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).
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/iceberg/service/IcebergRESTUtils.java:
##########
@@ -115,6 +115,16 @@ public static <T> T cloneIcebergRESTObject(Object message,
Class<T> className) {
}
}
+ 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) {
Review 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.
##########
iceberg/iceberg-rest-server/src/main/java/org/apache/gravitino/listener/api/event/IcebergReplaceViewEvent.java:
##########
@@ -19,36 +19,33 @@
package org.apache.gravitino.listener.api.event;
+import java.util.Map;
import org.apache.gravitino.NameIdentifier;
import org.apache.gravitino.annotation.DeveloperApi;
import org.apache.gravitino.iceberg.service.IcebergRESTUtils;
-import org.apache.iceberg.rest.requests.UpdateTableRequest;
-import org.apache.iceberg.rest.responses.LoadViewResponse;
/** Represent an event after updating Iceberg view successfully. */
@DeveloperApi
public class IcebergReplaceViewEvent extends IcebergViewEvent {
- private final UpdateTableRequest replaceViewRequest;
- private final LoadViewResponse loadViewResponse;
+ private final Map<String, Object> replaceViewRequest;
+ private final Map<String, Object> loadViewResponse;
public IcebergReplaceViewEvent(
IcebergRequestContext icebergRequestContext,
NameIdentifier viewIdentifier,
- UpdateTableRequest replaceViewRequest,
- LoadViewResponse loadViewResponse) {
+ Object replaceViewRequest,
+ Object loadViewResponse) {
super(icebergRequestContext, viewIdentifier);
- this.replaceViewRequest =
- IcebergRESTUtils.cloneIcebergRESTObject(replaceViewRequest,
UpdateTableRequest.class);
- this.loadViewResponse =
- IcebergRESTUtils.cloneIcebergRESTObject(loadViewResponse,
LoadViewResponse.class);
+ this.replaceViewRequest =
IcebergRESTUtils.toSerializableMap(replaceViewRequest);
+ this.loadViewResponse =
IcebergRESTUtils.toSerializableMap(loadViewResponse);
}
- public UpdateTableRequest renameViewRequest() {
+ public Map<String, Object> renameViewRequest() {
return replaceViewRequest;
}
Review 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).
```suggestion
/**
* 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();
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
