evindj commented on code in PR #3456:
URL: https://github.com/apache/polaris/pull/3456#discussion_r2699259570
##########
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:
How does this code ensures that map between loadTableResponses[i] maps to
tableChanges of i ?
Is there a scenario where they might not match?
##########
runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/CommitTransactionEventTest.java:
##########
@@ -117,6 +117,33 @@ void testEventsForUnSuccessfulTransaction() {
.isInstanceOf(IllegalStateException.class);
}
+ @Test
+ void testLoadTableResponsesInCommitTransaction() {
+ TestServices testServices = createTestServices();
+ createCatalogAndNamespace(testServices, Map.of(), catalogLocation);
+
+ String table1Name = "test-table-5";
+ String table2Name = "test-table-6";
+ executeTransactionTest(false, table1Name, table2Name, testServices);
+
+ TestPolarisEventListener testEventListener =
+ (TestPolarisEventListener) testServices.polarisEventListener();
+
+ // Verify that AfterUpdateTable events contain LoadTableResponse objects
+ PolarisEvent afterUpdateTableEvent =
+ testEventListener.getLatest(PolarisEventType.AFTER_UPDATE_TABLE);
Review Comment:
No assertion here?
##########
runtime/service/src/test/java/org/apache/polaris/service/catalog/iceberg/CommitTransactionEventTest.java:
##########
@@ -117,6 +117,33 @@ void testEventsForUnSuccessfulTransaction() {
.isInstanceOf(IllegalStateException.class);
}
+ @Test
+ void testLoadTableResponsesInCommitTransaction() {
+ TestServices testServices = createTestServices();
+ createCatalogAndNamespace(testServices, Map.of(), catalogLocation);
+
+ String table1Name = "test-table-5";
+ String table2Name = "test-table-6";
+ executeTransactionTest(false, table1Name, table2Name, testServices);
+
+ TestPolarisEventListener testEventListener =
+ (TestPolarisEventListener) testServices.polarisEventListener();
+
+ // Verify that AfterUpdateTable events contain LoadTableResponse objects
+ PolarisEvent afterUpdateTableEvent =
+ testEventListener.getLatest(PolarisEventType.AFTER_UPDATE_TABLE);
+
+ // Verify second table's LoadTableResponse
Review Comment:
probably a n00b question but my understanding was that two events will be
generated if that is the case, should you also test the first table?
--
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]
