github-actions[bot] commented on PR #61513: URL: https://github.com/apache/doris/pull/61513#issuecomment-4156553820
## Code Review Summary Reviewed all 13 files (+577/-11 lines). The PR fixes Paimon JDBC catalog system table queries that fail on BE because the JDBC driver is not available during JNI scanning. The fix propagates JDBC driver metadata (`jdbc.driver_url`, `jdbc.driver_class`) from FE to BE via a new Thrift field, then registers the driver before Paimon table initialization. ### Critical Checkpoint Conclusions **Goal & correctness**: The code accomplishes its stated goal. The end-to-end key naming chain (FE `getBackendPaimonOptions()` → Thrift `paimon_options` → BE C++ prefix with `paimon.` → Java `PaimonJdbcDriverUtils` lookup) is consistent and correct. Regression test, FE unit tests, and BE unit test all cover the new path. **Minimality & focus**: The change is well-scoped. Each file has a clear role in the fix. No unrelated changes. **Concurrency**: `JniScannerClassLoader.addURLIfAbsent()` is properly `synchronized`. `JdbcDriverUtils` uses `ConcurrentHashMap` for `DRIVER_CLASS_LOADER_CACHE` and `REGISTERED_DRIVER_KEYS`. The `REGISTERED_DRIVER_KEYS.add()` check-then-act has a small TOCTOU window but is benign (worst case: duplicate `DriverManager.registerDriver`, which is harmless). **Lifecycle / static init**: No cross-TU static initialization issues. Static caches (`DRIVER_CLASS_LOADER_CACHE`, `REGISTERED_DRIVER_KEYS`) have simple lifecycle with no circular references. **Compatibility (rolling upgrade)**: New Thrift field 30 (`paimon_options`) is `optional`. Both `paimon_cpp_reader.cpp` and `paimon_jni_reader.cpp` have proper fallback to per-split `paimon_params.paimon_options` when the scan-level field is absent. Mixed-version upgrades are safe. **Parallel code paths**: Both `paimon_cpp_reader.cpp` and `paimon_jni_reader.cpp` are updated with the same scan-level-then-split-level fallback pattern. No missed paths. **Configuration**: No new config items added. **FE-BE variable passing**: The new `paimon_options` is set at the scan-range-params level (not per-split), which is the correct granularity since JDBC driver info is catalog-wide. **Test coverage**: Good coverage — FE unit tests for `PaimonScanNode` and `PaimonJdbcMetaStoreProperties`, BE-side `PaimonJdbcDriverUtilsTest`, and regression test extension. One minor gap: `PaimonJdbcDriverUtilsTest.tearDown()` cleans `DriverManager` and temp files but does not clear `JdbcDriverUtils.REGISTERED_DRIVER_KEYS`, which could cause silent test failures if a second test registers the same driver key. Not a blocking issue since only one test currently registers a driver. **Observability**: `LOG.info` in `JdbcDriverUtils.registerDriver()` logs driver registration. Sufficient for debugging. **Performance**: No hot-path concerns. Driver registration is a one-time startup cost, properly cached with `REGISTERED_DRIVER_KEYS` dedup guard. **Error handling**: Status checks in C++ code are proper. Java exceptions propagate correctly. `JdbcDriverUtils.registerDriver()` handles `MalformedURLException`, `ClassNotFoundException`, `SQLException` with clear error messages including the driver URL. ### Minor Observations (Non-blocking) 1. **Code duplication**: `DriverShim` inner class and driver registration logic now exist in 3 places: `JdbcDriverUtils` (BE Java), `PaimonJdbcMetaStoreProperties` (FE), and `IcebergJdbcMetaStoreProperties` (FE). The FE-side duplication is a pre-existing pattern, and FE/BE are separate JVMs so cannot share directly. Consider extracting a shared utility within FE in a follow-up. 2. **Test cleanup**: `PaimonJdbcDriverUtilsTest.tearDown()` should also clear `JdbcDriverUtils.REGISTERED_DRIVER_KEYS` (e.g., via a `@VisibleForTesting` reset method) to ensure test isolation if more tests are added later. ### Verdict No blocking issues. The fix is architecturally sound, correctly propagates JDBC driver metadata through the FE→Thrift→BE chain, and has adequate test coverage. -- 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] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
