[Tech Debt] Avoid using reflection to load implementation classes of resources during the compilation phase#879
Conversation
…Python resource types
| } | ||
|
|
||
| private boolean isPythonResource(ResourceDescriptor descriptor) { | ||
| String pythonClazz = descriptor.getArgument("pythonClazz"); |
There was a problem hiding this comment.
Non-blocking:
The old check identified Python resources by whether clazz was a PythonResourceWrapper, while this now uses the presence of pythonClazz. Since PythonResourceProvider.provide() does not require pythonClazz when module is non-empty, do we want to document the assumption that descriptors reaching this classification path will always carry pythonClazz when they should be treated as Python?
There was a problem hiding this comment.
Thanks for your review! I think we should align on a single rule here. Since cross-language Python resources are required to declare pythonClazz, we probably should not leave other entry points open. Today, descriptors that take those alternate paths (e.g. non-empty module without pythonClazz) are classified as Java-side resources at plan-compilation time and fail at runtime. Tightening or closing those paths feels like a separate follow-up issue rather than something we need to solve in this PR—what do you think?
There was a problem hiding this comment.
Agreed. Making pythonClazz the single rule and closing the alternate paths is cleaner than documenting the assumption, and it belongs in its own change. Nit: those descriptors classify as Java at compile time and only fail at runtime, so sharpening that into a clearer compile-time error would be a good tracker. Otherwise LGTM.
There was a problem hiding this comment.
Given that normal Java AgentPlan paths don’t appear to produce module != empty && pythonClazz missing (module always be ""), I don’t think we need to preserve that old descriptor shape here. It seems limited to manually constructed 3-arg descriptors, so documenting/tightening that contract separately should be enough.
|
@wenjin272 @weiqingy Thanks for your review! I’ve updated the submission. PTAL~ |
Linked issue: #864
Purpose of change
Avoid using reflection to load implementation classes of resources during the compilation phase
Tests
ut test
API
no API
Documentation
doc-neededdoc-not-neededdoc-included