DanielLeens commented on PR #12626: URL: https://github.com/apache/seatunnel/pull/12626#issuecomment-6466883706
Thanks @SEZ9 for the thorough review. I rechecked the code at head `125cbbad4` and my earlier pass (which found no source blocker) was too lenient on several of these, so here is where I land on each. As this branch is my own, a follow-up commit is required for the valid ones; nothing below is fixed yet. - **Issue 1 (valid):** `findVisibleClass` resolves through `Class.forName(name, false, released)`, and `com.mongodb` is child-first, so the class is defined in the loader we are discarding. The static `Field.get` on `DEFAULT` (`ReleasedClassLoaderReferenceCleaner.java:127`) then runs `<clinit>`, which starts the pruner thread that `disablePruning()` immediately stops. That is wasted work under the service-wide `releaseClassLoader` monitor for jobs that never touched the driver. I will gate this step on evidence that the pool was really initialized instead of forcing it. - **Issue 2 (valid):** only `ReflectiveOperationException | RuntimeException | LinkageError` is caught, while the call at `DefaultClassLoaderService.java:146` sits after the maps are already cleaned and before the `isEmpty()` bookkeeping. An `Error` escaping there would abort the loop over the remaining jar sets in `recycleClassLoader`. The fix is to make `clean()` truly best-effort per step and to guard the call site so the service state is always consistent. - **Issue 3 (valid):** the tests only run against hand-written fixtures. I will add a contract check against the real driver / Hadoop classes in a module that has them on its classpath, so a dependency bump fails loudly. - **Issue 4 (agreed):** I will record the verified versions (mongodb-driver-core 4.7.1, Hadoop 3.1.4/3.3.6 uber) in the Javadoc and add pointer comments next to the version properties. I will also note the connector-owned release hook as the intended longer-term home. I will need a maintainer's view on whether that follow-up is required before merge. - **Issues 5, 6, 7 (low):** the registry iteration lock and re-checking `getConf` before `setConf(null)`, once-per-mismatch WARN logging with an operator switch, and the `entrie(s)` wording are all reasonable and cheap, so I will fold them into the same commit. I will push one commit covering these and post here when it is up, so it can be re-reviewed against the new head. -- 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]
