zhanglistar commented on code in PR #12297:
URL: https://github.com/apache/gluten/pull/12297#discussion_r3489018025
##########
gluten-flink/runtime/src/main/java/org/apache/gluten/velox/VeloxSourceSinkFactory.java:
##########
@@ -43,21 +58,67 @@ Transformation<RowData> buildVeloxSink(
/** Choose the matched source/sink factory by given transformation. */
private static Optional<VeloxSourceSinkFactory> getFactory(
- Transformation<RowData> transformation) {
+ Transformation<RowData> transformation, Map<String, Object> parameters) {
ServiceLoader<VeloxSourceSinkFactory> factories =
ServiceLoader.load(VeloxSourceSinkFactory.class);
- for (VeloxSourceSinkFactory factory : factories) {
- if (factory.match(transformation)) {
- return Optional.of(factory);
+ try {
+ for (VeloxSourceSinkFactory factory : factories) {
+ if (factory.match(transformation)) {
+ return Optional.of(factory);
+ }
+ }
+ } catch (ServiceConfigurationError e) {
+ LOG.warn("Failed to load Velox source/sink factory", e);
+ }
+ for (String factoryClassName : FACTORY_CLASS_NAMES) {
+ Optional<VeloxSourceSinkFactory> factory = loadFactory(factoryClassName,
parameters);
Review Comment:
Yes, The FACTORY_CLASS_NAMES list and the SPI file register exactly the same
7 factories. getFactory() first attempts ServiceLoader (SPI), then falls back
to manual class loading from FACTORY_CLASS_NAMES. This duplication is
intentional — runtime modules cannot depend on planner module classes at
compile time (there is a risk of circular dependencies), and the manual
loadFactory explicitly tries three different class loaders (from the parameter,
the thread context, and the interface's own loader), which is more robust than
ServiceLoader's single-loader approach in Flink's class loader hierarchy.
If this is to be cleaned up in the future, the safer direction is to keep
the FACTORY_CLASS_NAMES manual loading and remove the SPI file, because the
manual path handles class loaders more robustly. However, this refactoring is
outside the scope of this PR and should be verified in a real Flink deployment.
For now, keep it as-is.
--
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]
