yuqi1129 commented on PR #13672: URL: https://github.com/apache/gravitino/pull/13672#issuecomment-6009101243
Reviewed head `641beb98db3a7316ebdf4f1174912b8faa4d2987`. I found no behavioral difference in the current mappings, but recommend adding regression coverage before merging. 1. **[Important] Five specialized mapping tables are not protected by the unit suite.** [FunctionErrorHandler](https://github.com/apache/gravitino/blob/641beb98db3a7316ebdf4f1174912b8faa4d2987/clients/client-python/gravitino/exceptions/handlers/function_error_handler.py#L33), and the corresponding Model, Job, Tag, and Secret handlers. This PR rewrites their exception dispatch, but existing tests do not reliably exercise those mappings. For example, the [function exception test](https://github.com/apache/gravitino/blob/641beb98db3a7316ebdf4f1174912b8faa4d2987/clients/client-python/tests/unittests/test_function_catalog.py#L118-L126) makes the mocked `HTTPClient.get()` raise the expected exception directly, bypassing the error handler. I confirmed this with mutation testing: clearing `_code_exception_map` on all five handler singletons (function, model, job, tag, secret) in an isolated test process still leaves all **1,172 unit tests passing**. Losing specialized exceptions and falling back to generic exceptions would therefore go undetected by this suite. Please add parameterized tests with independently defined expected exceptions, covering specialized mappings, known-code/unknown-type fallback, unknown codes, and messages containing server stack entries. Also retain at least one test entering the handler through an HTTP error response. Avoid deriving the expected exception directly from the mapping under test. 2. **[Nit, non-blocking] The Pylint configuration changes are an additional scope change.** [pylintrc](https://github.com/apache/gravitino/blob/641beb98db3a7316ebdf4f1174912b8faa4d2987/clients/client-python/pylintrc#L101-L105) This also re-enables `too-many-arguments` and `too-many-positional-arguments` across the Python module with thresholds of 10. Please consider separating this policy change, or mention it explicitly in the PR description. Keeping it here is acceptable: the full Pylint check passes and it does not change runtime behavior. **Validation:** On Python 3.12.13, all 1,172 unit tests passed, and Pylint 4.0.5 passed against `gravitino`, `tests`, and `scripts`. The shared REST handler implementation has 100% statement and branch coverage (all eight branches), although that does not establish coverage of each specialized mapping table. I also compared the old and new implementations across all 19 handlers, 17 error codes, 70 exception-type strings, and three stack variants: **67,830 cases, zero differences** in exception type/message/arguments. The source checkout remained unchanged; mutations were confined to an isolated process. These were direct Python commands targeting the same unittest discovery and Pylint inputs as the Gradle tasks. Integration tests, the Python 3.10/3.11 matrix, real-server behavior, and external custom subclasses were not tested. -- 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]
