ss666 commented on PR #12571: URL: https://github.com/apache/gravitino/pull/12571#issuecomment-5426404219
@zhang-arvin Thanks for your effort! This is a great PR! I have a few comments below for your consideration. Hope they’re helpful. ### 1. Checked exception in `doNothing()` stubbing `CatalogDispatcher.testConnection(...)` declares `throws Exception`, but `mockCatalogDispatcher()` neither catches nor declares this checked exception. Therefore, this `doNothing().when(dispatcher).testConnection(...)` call causes a compilation error. Also, this stubbing does not seem to be needed because Mockito mocks already do nothing by default for `void` methods. Removing this stub entirely is the simplest fix. ### 2. `TEST_CONNECTION` is not integrated with the audit operation mapping Adding `OperationType.TEST_CONNECTION` requires corresponding mappings in both the v1 and v2 audit paths. Currently, there is no `AuditLog.Operation.TEST_CONNECTION` / `AuditLog.Operation.fromEvent(...)` mapping, and `CompatibilityUtils.OPERATION_TYPE_MAP` also does not contain `OperationType.TEST_CONNECTION`. As a result, the new connection-test events will be recorded as `UNKNOWN_OPERATION` instead of `TEST_CONNECTION`. The existing `testAllKnownOperationTypesMapToConcreteAuditOperation()` test will also fail because it requires every known `OperationType` to map to a concrete audit operation. Please add the corresponding audit operation, v1 `fromEvent(...)` mapping, v2 `CompatibilityUtils` mapping, and the relevant test case. ### 3. The existing-catalog `testConnection(NameIdentifier)` overload is still not implemented The single-argument overload `testConnection(NameIdentifier ident)` is left untouched, with the original TODO still in place. Please mirror the same pre / success / failure structure in `testConnection(NameIdentifier ident)`. Tests need to follow. ### 4. Formatting There is a formatting issue in `CatalogEventDispatcher.testConnection(...)` where the first `eventBus.dispatchEvent(...)` line is not indented consistently with the surrounding code. Please run the Spotless formatter before merging, referring to [Formatting Code with Spotless](https://github.com/apache/gravitino/blob/main/CONTRIBUTING.md#-formatting-code-with-spotless). -- 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]
