chesnokoff commented on code in PR #13540:
URL: https://github.com/apache/ignite/pull/13540#discussion_r4003825525
##########
modules/compatibility/src/test/java/org/apache/ignite/compatibility/testframework/testcontainers/IgniteContainer.java:
##########
@@ -383,77 +398,194 @@ private String execControl(String... cmd) {
return result.getStdout();
}
- /** @return Jar with {@link #TEST_CLASSES}, built once and reused for all
containers. */
- private static File testClassesJar() throws IOException {
- File jar = testClassesJar;
+ /**
+ * Builds the {@code control.sh} command line to be executed inside the
container.
+ *
+ * @param cmd Control utility arguments (e.g. {@code --set-state ACTIVE
--yes}).
+ * @return Full command whose first element is the absolute path to {@code
control.sh}, followed by {@code cmd}.
+ */
+ protected String[] command(String... cmd) {
+ String[] fullCmd = new String[cmd.length + 1];
+
+ fullCmd[0] = rootDir + "bin/control.sh";
+
+ System.arraycopy(cmd, 0, fullCmd, 1, cmd.length);
+
+ return fullCmd;
+ }
Review Comment:
Do we need both extension points? Overriding command() seems enough to
customize the command while keeping execution and error handling in the base
class. What use case requires execControl() to be protected?
##########
modules/compatibility/src/test/java/org/apache/ignite/compatibility/testframework/testcontainers/IgniteClusterContainer.java:
##########
@@ -30,24 +31,66 @@ public class IgniteClusterContainer implements Startable {
private final List<IgniteContainer> containers;
/** Network. */
- private final Network net = Network.newNetwork();
+ protected final Network net = Network.newNetwork();
+
+ /** Image name. */
+ protected final String imageName;
+
+ /** Consistent ID's. */
+ protected final List<String> consistentIds;
+
+ /** Whether the cluster has been started, guarding against a second {@link
#start()}. */
+ private boolean started;
/**
* @param imageName Image name.
* @param consistentIds Consistent ID's.
*/
- public IgniteClusterContainer(String imageName, List<String>
consistentIds) throws Exception {
+ public IgniteClusterContainer(String imageName, List<String>
consistentIds) {
+ this.imageName = imageName;
+ this.consistentIds = consistentIds;
+
containers = new ArrayList<>(consistentIds.size());
+ }
+
+ /**
+ * Factory hook for the node container. Overrides only receive {@code
idx}; the image name, network and
+ * consistent IDs are instance fields (see {@link #imageName}, {@link
#net}, {@link #consistentIds}).
+ *
+ * @param idx Node index.
+ * @return The node container.
+ */
+ protected IgniteContainer container(int idx) throws Exception {
+ return new IgniteContainer(imageName, net, "node" + (1 + idx),
consistentIds.get(idx), idx);
Review Comment:
Why do we do 1+idx here?
##########
modules/compatibility/src/test/java/org/apache/ignite/compatibility/testframework/testcontainers/IgniteClusterContainer.java:
##########
@@ -30,24 +31,66 @@ public class IgniteClusterContainer implements Startable {
private final List<IgniteContainer> containers;
/** Network. */
- private final Network net = Network.newNetwork();
+ protected final Network net = Network.newNetwork();
+
+ /** Image name. */
+ protected final String imageName;
+
+ /** Consistent ID's. */
+ protected final List<String> consistentIds;
+
+ /** Whether the cluster has been started, guarding against a second {@link
#start()}. */
+ private boolean started;
/**
* @param imageName Image name.
* @param consistentIds Consistent ID's.
*/
- public IgniteClusterContainer(String imageName, List<String>
consistentIds) throws Exception {
+ public IgniteClusterContainer(String imageName, List<String>
consistentIds) {
+ this.imageName = imageName;
+ this.consistentIds = consistentIds;
+
containers = new ArrayList<>(consistentIds.size());
+ }
+
+ /**
+ * Factory hook for the node container. Overrides only receive {@code
idx}; the image name, network and
+ * consistent IDs are instance fields (see {@link #imageName}, {@link
#net}, {@link #consistentIds}).
+ *
+ * @param idx Node index.
+ * @return The node container.
+ */
+ protected IgniteContainer container(int idx) throws Exception {
+ return new IgniteContainer(imageName, net, "node" + (1 + idx),
consistentIds.get(idx), idx);
+ }
+ /** Builds the node containers. */
+ protected void initContainers() throws Exception {
for (int i = 0; i < consistentIds.size(); i++)
- containers.add(new IgniteContainer(imageName, net, "node" + (1 +
i), consistentIds.get(i), i));
+ containers.add(container(i));
}
/** {@inheritDoc} */
@Override public void start() {
+ // Idempotent: either the cluster already started successfully, or
container creation succeeded
+ // but startup (deepStart/activateCluster) failed on a previous
attempt — in both cases the
+ // containers list is already populated and must not be built a second
time (duplicate hostnames,
+ // consistent IDs and fixed host ports would make the baseline
unreachable).
+ if (started || !containers.isEmpty())
+ return;
Review Comment:
Question: I agree that we should not call initContainers() twice. But should
we just return when `containers` is not empty and `started` is still false?
I'm not sure what is better here: silently skip it or throw an error
##########
modules/compatibility/src/test/java/org/apache/ignite/compatibility/testframework/testcontainers/IgniteContainer.java:
##########
@@ -383,77 +398,194 @@ private String execControl(String... cmd) {
return result.getStdout();
}
- /** @return Jar with {@link #TEST_CLASSES}, built once and reused for all
containers. */
- private static File testClassesJar() throws IOException {
- File jar = testClassesJar;
+ /**
+ * Builds the {@code control.sh} command line to be executed inside the
container.
+ *
+ * @param cmd Control utility arguments (e.g. {@code --set-state ACTIVE
--yes}).
+ * @return Full command whose first element is the absolute path to {@code
control.sh}, followed by {@code cmd}.
+ */
+ protected String[] command(String... cmd) {
+ String[] fullCmd = new String[cmd.length + 1];
+
+ fullCmd[0] = rootDir + "bin/control.sh";
+
+ System.arraycopy(cmd, 0, fullCmd, 1, cmd.length);
+
+ return fullCmd;
+ }
+
+ /** @return Classpath resource of the common (shared) node config copied
into the container. */
+ protected String commonConfigResource() {
+ return "docker/common-test-config.xml";
+ }
+
+ /** @return Classpath resource of the source (pre-upgrade) node config
copied into the container. */
+ protected String sourceConfigResource() {
+ return "docker/test-config.xml";
+ }
+
+ /** @return Classpath resource of the node config used on the target
(upgraded) side during in-place Docker upgrade. */
+ protected String targetConfigResource() {
+ return "docker/target-test-config.xml";
+ }
+
+ /** @return Ignite root directory inside the container, with a trailing
slash. */
+ protected String rootDirPath() {
+ return "/opt/ignite/apache-ignite/";
+ }
+
+ /** @return Custom classes (with their nested classes) used by the node in
containers. */
+ protected List<String> testClasses() {
+ return List.of(
+ ContainerAddressResolver.class.getName(),
+ TestCompatibilityPluginProvider.class.getName(),
+ DisabledRollingUpgradeProcessor.class.getName(),
+ DisabledValidationProcessor.class.getName()
+ );
+ }
+
+ /**
+ * @return Jar with the {@link #testClasses() test classes}, built per
distinct class list and reused.
+ * The cache is keyed on the effective {@link #testClasses()} result
so subclasses overriding it get
+ * their own jar instead of silently reusing the one built for the
base class.
+ */
+ protected File testClassesJar() throws IOException {
+ // List.copyOf makes an immutable, value-comparable key.
+ List<String> classes = List.copyOf(testClasses());
+
+ File jar = TEST_CLASSES_JARS.get(classes);
if (jar != null)
return jar;
synchronized (IgniteContainer.class) {
- if (testClassesJar != null)
- return testClassesJar;
+ jar = TEST_CLASSES_JARS.get(classes);
+
+ if (jar != null)
+ return jar;
jar = File.createTempFile("test-classes", ".jar");
jar.deleteOnExit();
try (JarOutputStream out = new JarOutputStream(new
FileOutputStream(jar))) {
- for (String cls : TEST_CLASSES) {
+ for (String cls : classes) {
String clsPath = cls.replace('.', '/') + ".class";
- URL url =
IgniteContainer.class.getClassLoader().getResource(clsPath);
-
- if (url == null)
- throw new IOException("Class not found on classpath: "
+ clsPath);
+ // Include the class and its nested classes (e.g. the
provider's anonymous $1).
+ // Each entry carries the URL already resolved by
classResources(), avoiding a second
+ // getResource() for the same top-level class (which would
otherwise be hit twice).
+ for (ClassResource res : classResources(clsPath)) {
+ out.putNextEntry(new JarEntry(res.name));
- File dir;
+ try (InputStream in = res.url.openStream()) {
+ in.transferTo(out);
+ }
- try {
- dir = new File(url.toURI()).getParentFile();
- }
- catch (URISyntaxException e) {
- throw new IOException(e);
+ out.closeEntry();
}
+ }
+ }
- String pkg = clsPath.substring(0, clsPath.lastIndexOf('/')
+ 1);
- String simple = cls.substring(cls.lastIndexOf('.') + 1);
+ TEST_CLASSES_JARS.put(classes, jar);
- // Include the class and its nested classes (e.g. the
provider's anonymous $1).
- File[] clsFiles = dir.listFiles((d, name) ->
- name.equals(simple + ".class") ||
name.startsWith(simple + '$'));
+ return jar;
+ }
+ }
- if (clsFiles == null)
- throw new IOException("Cannot list class directory: "
+ dir);
+ /**
+ * Resolves the fully qualified resources (the top-level class plus its
nested classes, e.g. {@code Outer$1})
+ * for a class located either on the file system or inside a jar on the
classpath. Each returned element pairs
+ * the resource name with its already-resolved URL, so callers do not need
to call {@code getResource()} again.
+ *
+ * @param clsPath Resource path of the top-level class (package separator
replaced with '/', ending in {@code .class}).
+ * @return Pairs of resource name and resolved URL for the class and its
nested classes.
+ */
+ private static Collection<ClassResource> classResources(String clsPath)
throws IOException {
+ String pkg = clsPath.substring(0, clsPath.lastIndexOf('/') + 1);
+ String simple = clsPath.substring(clsPath.lastIndexOf('/') + 1,
clsPath.length() - ".class".length());
+ String nestedPrefix = pkg + simple + "$";
- for (File f : clsFiles) {
- out.putNextEntry(new JarEntry(pkg + f.getName()));
+ ClassLoader cl = IgniteContainer.class.getClassLoader();
- Files.copy(f.toPath(), out);
+ URL url = cl.getResource(clsPath);
- out.closeEntry();
+ if (url == null)
+ throw new IOException("Class not found on classpath: " + clsPath);
+
+ List<ClassResource> res = new ArrayList<>();
+
+ try {
+ if ("file".equals(url.getProtocol())) {
+ File dir = new File(url.toURI()).getParentFile();
+
+ File[] clsFiles = dir.listFiles((d, name) ->
+ name.startsWith(simple + '$') && name.endsWith(".class"));
+
+ if (clsFiles == null)
+ throw new IOException("Cannot list class directory: " +
dir);
+
+ res.add(new ClassResource(clsPath, url));
+
+ for (File f : clsFiles)
+ res.add(new ClassResource(pkg + f.getName(),
cl.getResource(pkg + f.getName())));
+ }
+ else if ("jar".equals(url.getProtocol())) {
+ JarURLConnection conn = (JarURLConnection)url.openConnection();
+
+ try (JarFile jar = conn.getJarFile()) {
+ Enumeration<JarEntry> entries = jar.entries();
+
+ while (entries.hasMoreElements()) {
+ String name = entries.nextElement().getName();
+
+ if (name.equals(clsPath) ||
(name.startsWith(nestedPrefix) && name.endsWith(".class")))
+ res.add(new ClassResource(name,
cl.getResource(name)));
}
}
}
+ else
+ throw new IOException("Unsupported class resource protocol: "
+ url.getProtocol());
+ }
+ catch (URISyntaxException e) {
+ throw new IOException(e);
+ }
- return testClassesJar = jar;
+ return res;
+ }
+
+ /** A class resource: its name on the classpath paired with the
already-resolved URL. */
+ private static final class ClassResource {
+ /** Resource name on the classpath. */
+ final String name;
+
+ /** Resolved URL of the resource. */
+ final URL url;
+
+ /** @param name Resource name on the classpath. */
+ ClassResource(String name, URL url) {
+ this.name = name;
+ this.url = url;
}
}
Review Comment:
Probably we could use `record` to reduce LOC, but it's up to you
##########
modules/compatibility/src/test/java/org/apache/ignite/compatibility/testframework/testcontainers/IgniteContainer.java:
##########
@@ -383,77 +398,194 @@ private String execControl(String... cmd) {
return result.getStdout();
}
- /** @return Jar with {@link #TEST_CLASSES}, built once and reused for all
containers. */
- private static File testClassesJar() throws IOException {
- File jar = testClassesJar;
+ /**
+ * Builds the {@code control.sh} command line to be executed inside the
container.
+ *
+ * @param cmd Control utility arguments (e.g. {@code --set-state ACTIVE
--yes}).
+ * @return Full command whose first element is the absolute path to {@code
control.sh}, followed by {@code cmd}.
+ */
+ protected String[] command(String... cmd) {
+ String[] fullCmd = new String[cmd.length + 1];
+
+ fullCmd[0] = rootDir + "bin/control.sh";
+
+ System.arraycopy(cmd, 0, fullCmd, 1, cmd.length);
+
+ return fullCmd;
+ }
+
+ /** @return Classpath resource of the common (shared) node config copied
into the container. */
+ protected String commonConfigResource() {
+ return "docker/common-test-config.xml";
+ }
+
+ /** @return Classpath resource of the source (pre-upgrade) node config
copied into the container. */
+ protected String sourceConfigResource() {
+ return "docker/test-config.xml";
+ }
+
+ /** @return Classpath resource of the node config used on the target
(upgraded) side during in-place Docker upgrade. */
+ protected String targetConfigResource() {
+ return "docker/target-test-config.xml";
+ }
+
+ /** @return Ignite root directory inside the container, with a trailing
slash. */
+ protected String rootDirPath() {
+ return "/opt/ignite/apache-ignite/";
+ }
+
+ /** @return Custom classes (with their nested classes) used by the node in
containers. */
+ protected List<String> testClasses() {
+ return List.of(
+ ContainerAddressResolver.class.getName(),
+ TestCompatibilityPluginProvider.class.getName(),
+ DisabledRollingUpgradeProcessor.class.getName(),
+ DisabledValidationProcessor.class.getName()
+ );
+ }
+
+ /**
+ * @return Jar with the {@link #testClasses() test classes}, built per
distinct class list and reused.
+ * The cache is keyed on the effective {@link #testClasses()} result
so subclasses overriding it get
+ * their own jar instead of silently reusing the one built for the
base class.
+ */
+ protected File testClassesJar() throws IOException {
+ // List.copyOf makes an immutable, value-comparable key.
+ List<String> classes = List.copyOf(testClasses());
Review Comment:
I'm not sure we need the comment. Looks like it just explains semantics of
`List.copyOf`
--
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]
