SAMZA-2354: Improve UDF discovery in samza-sql. #1192
Conversation
Replace ConfigBasedUDFResolver with the UDF resolver based on the reflections.
|
@atoomula @srinipunuru |
| @@ -1,119 +0,0 @@ | |||
| /* | |||
There was a problem hiding this comment.
Please don't delete this file.
There was a problem hiding this comment.
Sure, reverted this deletion.
| public ReflectionBasedUdfResolver(Config udfConfig) { | ||
| // Searching the entire classpath to discover the subtypes of SamzaSqlUdf is expensive. To reduce the search space, | ||
| // the search is limited to the set of package prefixes defined in the configuration. | ||
| // Within Linkedin this configuration will be overridden to ["com.linkedin.samza", "org.apache.samza", "com.linkedin.samza.sql.shade.prefix"]. |
There was a problem hiding this comment.
Please remove references to Linkedin from the code.
There was a problem hiding this comment.
Removed the LinkedIn comment.
| * ensure that all subtypes of {@link SamzaSqlUdf} extend and implement the | ||
| * method annotated with {@link SamzaSqlUdfMethod}. | ||
| */ | ||
| public class ReflectionBasedUdfResolver implements UdfResolver { |
There was a problem hiding this comment.
Just curious, how much of a hit are we taking with OSS jar to load the UDFs via reflection ?
There was a problem hiding this comment.
By selectively searching for UDF's only in selected package-prefixes from class-path for UDF's, it's a very minimal over-head.
| * ensure that all subtypes of {@link SamzaSqlUdf} extend and implement the | ||
| * method annotated with {@link SamzaSqlUdfMethod}. | ||
| */ | ||
| public class ReflectionBasedUdfResolver implements UdfResolver { |
There was a problem hiding this comment.
By selectively searching for UDF's only in selected package-prefixes from class-path for UDF's, it's a very minimal over-head.
| public ReflectionBasedUdfResolver(Config udfConfig) { | ||
| // Searching the entire classpath to discover the subtypes of SamzaSqlUdf is expensive. To reduce the search space, | ||
| // the search is limited to the set of package prefixes defined in the configuration. | ||
| // Within Linkedin this configuration will be overridden to ["com.linkedin.samza", "org.apache.samza", "com.linkedin.samza.sql.shade.prefix"]. |
There was a problem hiding this comment.
Removed the LinkedIn comment.
| @@ -1,119 +0,0 @@ | |||
| /* | |||
There was a problem hiding this comment.
Sure, reverted this deletion.
| @@ -214,7 +214,7 @@ private UdfResolver createUdfResolver(Map<String, String> config) { | |||
| Properties props = new Properties(); | |||
| props.putAll(domainConfig); | |||
| HashMap<String, String> udfConfig = getDomainProperties(config, CFG_UDF_CONFIG_DOMAIN, false); | |||
| return new ConfigBasedUdfResolver(props, new MapConfig(udfConfig)); | |||
| return new ReflectionBasedUdfResolver(new MapConfig(udfConfig)); | |||
There was a problem hiding this comment.
Sorry, I didn't realize that this is hard-coded here. Could we make this pluggable ? I think calling initializePlugin like above (createIOResolver method) should do it with the udf resolver based arguments.
There was a problem hiding this comment.
I agree that this is a useful thing to do and discussed it offline. Created a follow-up ticket(SAMZA-2355) for making the UDFResolver pluggable in samza-sql. It will be done in an immediate follow-up patch.
* Improve UDF discovery in the samza-sql. Replace ConfigBasedUDFResolver with the UDF resolver based on the reflections. * Address review comments. * Add TODO for the follow-up ticket in the comments.
This patch is comprised of following changes:
samza.sql.udfClassesconfiguration. Any user error made in defining the fully-qualified name of udf-class configuration can only be identified at run-time. This patch addresses this problem by using reflection to find all the samza-sql UDF implementations available in the class-path of the samza-sql application.UDFResolverimplementation.