wsxuehao commented on issue #12164:
URL: https://github.com/apache/gravitino/issues/12164#issuecomment-5187266059
Thanks for the detailed analysis. You are right, and I appreciate you
pointing out that the proposed code only addresses part of the problem.
The main case I would like this issue to address is the second one:
- both the server and the catalog contain the same dependency;
- the versions are different;
- the server version is loaded first;
- loading initially succeeds, so the existing fallback is not triggered;
- the incompatibility appears later as NoSuchMethodError, LinkageError, or
ClassCastException.
I agree that changing only the non-shared branch to call findClass() first
would not be sufficient. As you noted, the current isSharedClass() treats
classes outside isCatalogClass() as shared, so most catalog-private third-party
dependencies would still follow the parent-first path.
My intended model is:
- explicitly shared APIs are loaded from the parent classloader;
- catalog implementation classes and ordinary catalog-private dependencies
use child-first loading;
- private classes fall back to the parent only when they are not available
in the catalog package;
- the existing barrier-class behavior remains unchanged.
For explicitly shared APIs, it may be safer to require them to come from
the parent rather than falling back to a catalog-private copy, because loading
two definitions of a shared API could introduce ClassCastException or other
linkage issues.
The package-classification maintenance problem is also relevant, although
I agree that a missing package does not necessarily fail when the server does
not contain the class. The more problematic case is when a related or
conflicting class is already visible to the server: it may be loaded
successfully from the wrong classloader, so no fallback occurs.
Therefore, a complete solution would likely require both:
1. changing the loading order for private classes; and
2. changing the classification model from a catalog-package allowlist to a
shared-API allowlist, with other dependencies treated as private by default.
Since this may affect existing catalogs, starting with an opt-in
catalog-level child-first strategy sounds safer. A test using different
versions of the same dependency in the server and catalog classpaths would also
help verify the intended behavior.
Thanks again for the clarification. I can update the issue description to
make the intended scope and the limitation of the sample implementation clearer.
--
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]
