adutra commented on code in PR #3456:
URL: https://github.com/apache/polaris/pull/3456#discussion_r2709166219
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergRestCatalogEventServiceDelegator.java:
##########
Review Comment:
Shouldn't we change all these `new AttributeMap()` occurrences to use the
injected attribute map instead?
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##########
@@ -1092,6 +1098,10 @@ public void commitTransaction(CommitTransactionRequest
commitTransactionRequest)
if (!updatedMetadata.changes().isEmpty()) {
tableOps.commit(currentMetadata, updatedMetadata);
}
+
+ LoadTableResponse loadTableResponse =
+
LoadTableResponse.builder().withTableMetadata(updatedMetadata).build();
Review Comment:
We're creating a fake `LoadTableResponse` just for events. Couldn't we just
store the `TableMetadata` object?
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergRestCatalogEventServiceDelegator.java:
##########
@@ -783,7 +787,14 @@ public Response commitTransaction(
new AttributeMap()
.put(EventAttributes.CATALOG_NAME, catalogName)
.put(EventAttributes.COMMIT_TRANSACTION_REQUEST,
commitTransactionRequest)));
- for (UpdateTableRequest req : commitTransactionRequest.tableChanges()) {
+ List<LoadTableResponse> loadTableResponses =
+ attributeMap.getRequired(EventAttributes.LOAD_TABLE_RESPONSES);
+ for (int i = 0; i < commitTransactionRequest.tableChanges().size(); i++) {
+ UpdateTableRequest req = commitTransactionRequest.tableChanges().get(i);
+ LoadTableResponse loadTableResponse =
+ loadTableResponses != null && i < loadTableResponses.size()
+ ? loadTableResponses.get(i)
+ : null;
Review Comment:
The list of table responses is constructed in `IcebergCatalogHandler`:
https://github.com/apache/polaris/blob/b820bd09db3f67dae9623b9c9f0f19c9387e66d5/runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java#L1104
##########
runtime/service/src/main/java/org/apache/polaris/service/events/AttributeMap.java:
##########
@@ -18,11 +18,13 @@
*/
package org.apache.polaris.service.events;
+import jakarta.enterprise.context.RequestScoped;
import java.util.HashMap;
import java.util.Map;
import java.util.Optional;
/** A type-safe container for event attributes. This class is mutable and not
thread-safe! */
+@RequestScoped
Review Comment:
This is a good remark. Thanks to context propagation, the request context
"follows" the execution flow, even if the thread changes. You usually wouldn't
have race conditions, because two threads aren't accessing the bean
_simultaneously_, even if different threads access it _successively_.
But we would enter a danger zone if/when we spawn threads manually (e.g.
with the task executor), and pass this bean to more than one thread
concurrently.
I don't think this is happening today, but it could happen in the future.
I therefore think we should either make this bean thread-safe, or explicitly
call out in the class javadocs that it is not, and that it should not be passed
to the task executor framework.
##########
runtime/service/src/main/java/org/apache/polaris/service/catalog/iceberg/IcebergCatalogHandler.java:
##########
@@ -183,6 +185,7 @@ public class IcebergCatalogHandler extends CatalogHandler
implements AutoCloseab
private final ReservedProperties reservedProperties;
private final CatalogHandlerUtils catalogHandlerUtils;
private final StorageAccessConfigProvider storageAccessConfigProvider;
+ private final AttributeMap attributeMap;
Review Comment:
Yes, this bean was private to the events package before. I agree that it has
now a broader usage, it should be renamed.
--
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]
