[GitHub] [samza] mynameborat commented on a change in pull request #1350: SAMZA-2498: [Job coordinator isolation] Classloader isolation for SamzaApplication.describe execution in job coordinator

[GitBox <gi...@apache.org>]

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:
users@infra.apache.org