mynameborat commented on a change in pull request #1350:
URL: https://github.com/apache/samza/pull/1350#discussion_r425472665
##########
File path:
samza-core/src/main/java/org/apache/samza/classloader/IsolatingClassLoaderFactory.java
##########
@@ -76,16 +81,23 @@
* In order to share objects between classes loaded by different
classloaders, the classes for the shared objects must
* be loaded by a common classloader. Those common classes will be loaded
through a common API classloader. The
* cytodynamics classloader can be set up to only use the common API
classloader for an explicit set of classes. The
- * {@link DependencyIsolationUtils#FRAMEWORK_API_CLASS_LIST_FILE_NAME} file
should include the framework API classes.
- * Also, bootstrap classes (e.g. java.lang.String) need to be loaded by a
common classloader, since objects of those
- * types need to be shared across different framework and application. There
are also some static bootstrap classes
- * which should be shared (e.g. java.lang.System). Bootstrap classes will be
loaded through a common classloader by
- * default.
+ * {@link DependencyIsolationUtils#FRAMEWORK_API_CLASS_LIST_FILE_NAME} file
and the
+ * {@link DependencyIsolationUtils#FRAMEWORK_DESCRIPTORS_FILE_NAME} file
specify these classes. Also, bootstrap
+ * classes (e.g. java.lang.String) need to be loaded by a common
classloader, since objects of those types need to be
+ * shared across different framework and application. There are also some
static bootstrap classes which should be
+ * shared (e.g. java.lang.System). Bootstrap classes will be loaded through
a common classloader by default.
+ *
+ * The classes in {@link
DependencyIsolationUtils#FRAMEWORK_API_CLASS_LIST_FILE_NAME} and
+ * {@link DependencyIsolationUtils#FRAMEWORK_DESCRIPTORS_FILE_NAME} are
specified separately, because only the
+ * application classloader (not the infrastructure classloader) should
delegate to the framework API for the concrete
+ * descriptors and table functions (while describing the application). The
descriptor information will get serialized
+ * after application description and deserialized through the infrastructure
classloader, so the infrastructure can
+ * use them for message processing.
*
* These are the classloaders which are used to make up the final
classloader.
* <ul>
* <li>bootstrap classloader: Built-in Java classes (e.g.
java.lang.String)</li>
- * <li>API classloader: Common Samza framework API classes</li>
+ * <li>API classloader: Common Samza framework API classes and concrete
descriptors for application description</li>
Review comment:
Can you update the list item doc below on `Application classloader` to
include framework descriptors?
##########
File path:
samza-core/src/main/java/org/apache/samza/classloader/DependencyIsolationUtils.java
##########
@@ -42,9 +42,18 @@
/**
* Name of the file which contains the class names (or globs) which should
be loaded from the framework API
- * classloader.
+ * classloader by all classloaders.
+ * See {@link IsolatingClassLoaderFactory} for more context about what this
is used for.
*/
public static final String FRAMEWORK_API_CLASS_LIST_FILE_NAME =
"samza-framework-api-classes.txt";
+ /**
+ * Name of the file which contains the class names (or globs) of pluggable
components used during the application
+ * description step (e.g. system descriptors, table functions), which should
be loaded from the framework API
+ * classloader for the application classloader.
+ * See {@link IsolatingClassLoaderFactory} for more context about what this
is used for.
+ */
+ public static final String FRAMEWORK_DESCRIPTORS_FILE_NAME =
"samza-framework-descriptors-classes.txt";
Review comment:
nit: should we rename this to
`FRAMEWORK_DESCRIPTORS_CLASS_LIST_FILE_NAME` to be consistent with the
framework api constant above?
----------------------------------------------------------------
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.
For queries about this service, please contact Infrastructure at:
[email protected]
