adutra commented on code in PR #3456:
URL: https://github.com/apache/polaris/pull/3456#discussion_r2726758919
##########
runtime/service/src/main/java/org/apache/polaris/service/events/EventAttributes.java:
##########
@@ -108,7 +110,9 @@ private EventAttributes() {}
public static final AttributeKey<RenameTableRequest> RENAME_TABLE_REQUEST =
new AttributeKey<>("rename_table_request", RenameTableRequest.class);
public static final AttributeKey<LoadTableResponse> LOAD_TABLE_RESPONSE =
- new AttributeKey<>("load_table_response", LoadTableResponse.class);
+ new AttributeKey<>("load_table_responses", LoadTableResponse.class);
Review Comment:
The attribute name is `load_table_responses` in the plural, yet the
attribute seems to target a single response object.
##########
runtime/service/src/main/java/org/apache/polaris/service/events/EventAttributes.java:
##########
@@ -108,7 +110,9 @@ private EventAttributes() {}
public static final AttributeKey<RenameTableRequest> RENAME_TABLE_REQUEST =
new AttributeKey<>("rename_table_request", RenameTableRequest.class);
public static final AttributeKey<LoadTableResponse> LOAD_TABLE_RESPONSE =
- new AttributeKey<>("load_table_response", LoadTableResponse.class);
+ new AttributeKey<>("load_table_responses", LoadTableResponse.class);
+ public static final AttributeKey<List<TableMetadata>> TABLE_METADATA =
+ new AttributeKey<>("table_metadatas", new
TypeToken<List<TableMetadata>>() {});
Review Comment:
There seems to be an inconsistency here: sometimes this attribute seems to
be legitimately collecting a list of metadatas, cf. `IcebergCatalogHandler`.
But sometimes, it's collecting a singleton list of a unique table metadata, cf.
`IcebergCatalogEventServiceDelegator`.
I think it would be better to distinguish two attributes: `TABLE_METADATA`
and `TABLE_METADATAS`. (I understand that the latter may become a sort of
"internal" attribute that is never exposed to listeners – we could maybe
declare those separately if we are concerned about that.)
##########
runtime/service/src/main/java/org/apache/polaris/service/events/EventAttributes.java:
##########
@@ -108,7 +110,9 @@ private EventAttributes() {}
public static final AttributeKey<RenameTableRequest> RENAME_TABLE_REQUEST =
new AttributeKey<>("rename_table_request", RenameTableRequest.class);
public static final AttributeKey<LoadTableResponse> LOAD_TABLE_RESPONSE =
- new AttributeKey<>("load_table_response", LoadTableResponse.class);
+ new AttributeKey<>("load_table_responses", LoadTableResponse.class);
+ public static final AttributeKey<List<TableMetadata>> TABLE_METADATA =
+ new AttributeKey<>("table_metadatas", new
TypeToken<List<TableMetadata>>() {});
Review Comment:
nit: `new TypeToken<>(){}` is enough:
```suggestion
public static final AttributeKey<List<TableMetadata>> TABLE_METADATA =
new AttributeKey<>("table_metadatas", new TypeToken<>() {});
```
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergRestCatalogEventServiceDelegator.java:
##########
@@ -345,15 +350,22 @@ public Response loadTable(
snapshots,
realmContext,
securityContext);
+
+ EventAttributeMap generatedEventAttributes =
Review Comment:
This change doesn't seem necessary?
##########
runtime/service/src/main/java/org/apache/polaris/service/events/EventAttributes.java:
##########
@@ -108,7 +110,9 @@ private EventAttributes() {}
public static final AttributeKey<RenameTableRequest> RENAME_TABLE_REQUEST =
new AttributeKey<>("rename_table_request", RenameTableRequest.class);
public static final AttributeKey<LoadTableResponse> LOAD_TABLE_RESPONSE =
- new AttributeKey<>("load_table_response", LoadTableResponse.class);
+ new AttributeKey<>("load_table_responses", LoadTableResponse.class);
+ public static final AttributeKey<List<TableMetadata>> TABLE_METADATA =
+ new AttributeKey<>("table_metadatas", new
TypeToken<List<TableMetadata>>() {});
Review Comment:
The field name should reflect the attribute name: `TABLE_METADATAS`.
--
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]
