[GitHub] [druid] paul-rogers commented on a diff in pull request #13877: Extend the IT framework to allow tests in extensions

["paul-rogers (via GitHub)" <gi...@apache.org>]

paul-rogers commented on code in PR #13877:
URL: https://github.com/apache/druid/pull/13877#discussion_r1128914971


##########
integration-tests-ex/cases/cluster/template.py:
##########
@@ -49,16 +49,21 @@ def generate(template_path, template):
     '''
 
     # Compute the cluster (test category) name from the template path which
-    # we assume to be module/<something>/<template>/<something>.py
+    # we assume to be <module>/templates/<something>.py
     template_path = Path(template_path)
-    cluster = template_path.stem
+    cluster = template_path.parent.name
 
-    # Move up to the module (that is, the cases folder) relative to the template file.
-    module_dir = Path(__file__).parent.parent
+    # Move up to the module relative to the template file.
+    module_dir = template_path.parent.parent.parent
 
     # The target location for the output file is <module>/target/cluster/<cluster>/docker-compose.yaml
     target_dir = module_dir.joinpath("target")
+    os.makedirs(target_dir, exist_ok=True)
     target_file = target_dir.joinpath('cluster', cluster, 'docker-compose.yaml')
+    # Uncomment the following for debugging
+    #print("template_path:", template_path)

Review Comment:
   We could. But, they are only ever used when we change something. The user doesn't care: if they are wrong, things don't work, so they have to be right.
   
   To avoid a discussion, removed the lines.



##########
docs/configuration/index.md:
##########
@@ -122,6 +122,7 @@ Many of Druid's external dependencies can be plugged in as modules. Extensions c
 |`druid.extensions.useExtensionClassloaderFirst`|This is a boolean flag that determines if Druid extensions should prefer loading classes from their own jars rather than jars bundled with Druid. If false, extensions must be compatible with classes provided by any jars bundled with Druid. If true, extensions may depend on conflicting versions.|false|
 |`druid.extensions.hadoopContainerDruidClasspath`|Hadoop Indexing launches hadoop jobs and this configuration provides way to explicitly set the user classpath for the hadoop job. By default this is computed automatically by druid based on the druid process classpath and set of extensions. However, sometimes you might want to be explicit to resolve dependency conflicts between druid and hadoop.|null|
 |`druid.extensions.addExtensionsToHadoopContainer`|Only applicable if `druid.extensions.hadoopContainerDruidClasspath` is provided. If set to true, then extensions specified in the loadList are added to hadoop container classpath. Note that when `druid.extensions.hadoopContainerDruidClasspath` is not provided then extensions are always added to hadoop container classpath.|false|
+|`druid.extensions.path`|An optional array of paths to search for extensions. The `directory` entry acts as the first entry in the search path. Primarily for testing and Docker-based environments.|null (i.e. no additional search path)|

Review Comment:
   Here I'm following the normal [Lin|U]nix pattern. In Bash, we use `PATH` for the search path. In Python, it is `PYTHONPATH`. The path, by definition, is a list.
   
   Of course, now people say "file path" to mean a set of directories. I made that mistake here and changed it to "array of directories". But, I've not heard some one explain `PATH` as "command search paths" (with an "s").
   
   I _could_ change it to "additionalDirectoriesArray", but I feel that is overkill.
   
   Thoughts?



##########
processing/src/main/java/org/apache/druid/guice/ExtensionsConfig.java:
##########
@@ -20,35 +20,63 @@
 package org.apache.druid.guice;
 
 import com.fasterxml.jackson.annotation.JsonProperty;
+import com.google.common.collect.Sets;
 
+import javax.annotation.Nullable;
 import javax.validation.constraints.NotNull;
+
 import java.util.LinkedHashSet;
+import java.util.List;
 
 /**
+ * Configuration for Druid extensions.
  */
 public class ExtensionsConfig
 {
   public static final String PROPERTY_BASE = "druid.extensions";
+  public static final String DEFAULT_EXTENSIONS_DIR = "extensions";
 
   @JsonProperty
   @NotNull
   private boolean searchCurrentClassloader = true;
 
+  /**
+   * The location of the "primary" extensions directory. If the path is relative
+   * (as in the default), then the path is relative to the Druid directory
+   * (assuming the Druid product directory is indicated by the working directory.)
+   * <p>
+   * The value can be blank to indicate to ignore this value and only use the path.
+   * (If the value is omitted from the config file, it defaults to "extensions", so
+   * we need a blank value to say to ignore this property.)
+   */
+  @JsonProperty
+  private String directory = DEFAULT_EXTENSIONS_DIR;
+
+  /**
+   * Extensions path. Items may be relative to the Druid home directory, or absolute.
+   * Extensions are resolved in order along the path: the first match wins. If
+   * {@link #directory} is set, it becomes the first element on the full path.
+   * A typical use is to let {@code directory} point to {@code $DRUID_HOME/extensions},
+   * and use {@link path} to point to extra extensions mounted into a Docker container.
+   * Primarily for testing.
+   */
   @JsonProperty
-  private String directory = "extensions";
+  @Nullable
+  private List<String> path;

Review Comment:
   I see the point. We generally have documentation and examples since our property names don't include details. For example, `fooTimeout` often doesn't say `fooTimeoutMs`, `fooSize` doesn't say `fooSizeBytes` and `loadList` doesn't say `loadArrayOfString`.
   
   That said, I'm open to suggestions if we feel folks can't look up the field in docs or in an example.



##########
integration-tests-ex/docs/docker.md:
##########
@@ -211,6 +211,37 @@ when it starts. If you start, then restart the MySQL container, you *must*
 remove the `db` directory before restart or MySQL will fail due to existing
 files.
 
+### Per-test Extensions
+
+The image build includes a standard set of extensions. Contrib or custom extensions
+may wish to add additional extensions. This is most easily done not by altering the
+image, but by adding the extensions at cluster startup. If the shared directory has
+an `extensions` subdirectory, then that directory is added to the extension search
+path on container startup. To add an extension `my-extension`, your shared directory
+should look like this:
+
+```text
+shared
++- ...
++- extensions
+   +- my-extension
+      +- my-extension-<version>.jar
++- ...
+```
+
+The the `extensions` directory should be created within the per-cluster `setup.sh` script

Review Comment:
   Actually, it is repeated once, but appears twice. :-)



-- 
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: commits-unsubscribe@druid.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org


---------------------------------------------------------------------
To unsubscribe, e-mail: commits-unsubscribe@druid.apache.org
For additional commands, e-mail: commits-help@druid.apache.org