maksaska commented on code in PR #13540:
URL: https://github.com/apache/ignite/pull/13540#discussion_r3978779458
##########
modules/compatibility/src/test/java/org/apache/ignite/compatibility/testframework/testcontainers/IgniteContainer.java:
##########
@@ -142,29 +125,41 @@ public class IgniteContainer extends
GenericContainer<IgniteContainer> {
/** Consistent ID. */
private final String consistentId;
- /** Path to work directory. */
- private final String workDirPath;
+ /** Ignite root directory in container, computed from {@link
#rootDirPath()}. */
+ private final String rootDir;
+
+ /** Ignite libs directory in container. */
+ private final String libsDirPath;
+
+ /** Config path in container. */
+ private final String cfgPath;
/**
* @param imageName Image name.
* @param net Network.
* @param hostname Hostname.
* @param consistentId Consistent ID.
- * param idx Node index.
+ * @param idx Node index.
*/
public IgniteContainer(String imageName, Network net, String hostname,
String consistentId, int idx) throws Exception {
super(DockerImageName.parse(imageName));
this.hostname = hostname;
this.consistentId = consistentId;
- workDirPath = WORK_DIR_PATH + "/" + hostname;
+ rootDir = rootDirPath();
Review Comment:
The constructor calls the new overridable hooks before a subclass can
initialize its own state: rootDirPath() (line 149),
commonConfigResource()/sourceConfigResource() (lines 198-200), testClasses()
via testClassesJar() (line 200) and waitStrategy() (line 215).
Since extensibility is the point of the PR, could these be resolved lazily
on first use (or moved into configure() / containerIsCreating(), which
Testcontainers already provides for exactly this)? At minimum the javadoc
should state that overrides must not depend on subclass instance state.
##########
modules/compatibility/src/test/java/org/apache/ignite/compatibility/testframework/testcontainers/IgniteClusterContainer.java:
##########
@@ -30,21 +31,52 @@ public class IgniteClusterContainer implements Startable {
private final List<IgniteContainer> containers;
/** Network. */
- private final Network net = Network.newNetwork();
+ private final Network net;
+
+ /** Image name. */
+ private final String imageName;
+
+ /** Consistent ID's. */
+ private final List<String> consistentIds;
/**
* @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;
+
+ net = Network.newNetwork();
containers = new ArrayList<>(consistentIds.size());
+ }
+ /**
+ * @param imageName Image name.
+ * @param net Shared test network the container must be attached to.
+ * @param consistentIds Consistent ID's.
+ * @param idx Node index.
+ * @return The node container.
+ */
+ protected IgniteContainer container(String imageName, Network net,
List<String> consistentIds, int idx) throws Exception {
Review Comment:
container(String imageName, Network net, List<String> consistentIds, int
idx) re-passes imageName, net and consistentIds, all of which are already
fields of the instance, and hands the override the whole list plus an index
rather than the id it needs. protected IgniteContainer container(int idx) would
be a cleaner hook; if the parameters are kept for readability, passing
consistentIds.get(idx) instead of the list would still be an improvement.
##########
modules/compatibility/src/test/java/org/apache/ignite/compatibility/testframework/testcontainers/IgniteContainer.java:
##########
@@ -116,18 +107,10 @@ public class IgniteContainer extends
GenericContainer<IgniteContainer> {
/** Base host port for the published thin-client port (node index added).
*/
private static final int CLIENT_HOST_PORT_BASE = 50800;
- /** Custom classes (with their nested classes) used by node in containers.
*/
- private static final List<String> TEST_CLASSES = List.of(
- ContainerAddressResolver.class.getName(),
- TestCompatibilityPluginProvider.class.getName(),
- DisabledRollingUpgradeProcessor.class.getName(),
- DisabledValidationProcessor.class.getName()
- );
-
/** Seconds to wait after SIGTERM before SIGKILL. */
private static final int SHUTDOWN_TIMEOUT_SEC = 30;
- /** Jar holding {@link #TEST_CLASSES}, injected so the old image can load
it. */
+ /** Jar holding the {@link #testClasses() test classes}, injected so the
old image can load it. */
private static volatile File testClassesJar;
Review Comment:
testClasses() is now overridable per subclass, but the resulting jar is
still memoized in private static volatile File testClassesJar (line 114) with
no key. targetLibsArchive (line 117) / libsArchive() (line 534) have the same
shape, and the latter embeds that jar.
Within one surefire fork, whichever container is constructed first wins: if
the base test builds the jar with the four default classes and a subclass
overriding testClasses() runs afterwards, the subclass silently reuses the
stale jar and its node dies with ClassNotFoundException — with the outcome
depending on test execution order.
Suggest keying the cache on the effective class list (e.g.
ConcurrentHashMap<List<String>, File>) or making it instance-level.
##########
modules/compatibility/src/test/java/org/apache/ignite/compatibility/testframework/testcontainers/IgniteClusterContainer.java:
##########
@@ -30,21 +31,52 @@ public class IgniteClusterContainer implements Startable {
private final List<IgniteContainer> containers;
/** Network. */
- private final Network net = Network.newNetwork();
+ private final Network net;
+
+ /** Image name. */
+ private final String imageName;
+
+ /** Consistent ID's. */
+ private final List<String> consistentIds;
/**
* @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;
+
+ net = Network.newNetwork();
containers = new ArrayList<>(consistentIds.size());
+ }
+ /**
+ * @param imageName Image name.
+ * @param net Shared test network the container must be attached to.
+ * @param consistentIds Consistent ID's.
+ * @param idx Node index.
+ * @return The node container.
+ */
+ protected IgniteContainer container(String imageName, Network net,
List<String> consistentIds, 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 {
Review Comment:
Moving container creation from the constructor into initContainers() makes
start() non-idempotent: the method always appends to the pre-existing
containers list, so a second start() — or a retry after
Startables.deepStart(...).join() fails — yields 2N containers with duplicate
hostnames, duplicate consistent IDs and duplicate fixed host ports, after which
activateCluster(containers.size()) waits for a baseline size that can never be
reached.
Also, containers() used to be populated right after construction and now
returns an empty list until start() runs. Nothing in the current test depends
on that, but it is a silent contract change for subclasses that might want to
inspect containers before starting.
A if (!containers.isEmpty()) return; guard (or a started flag) would cover
both.
##########
modules/compatibility/src/test/java/org/apache/ignite/compatibility/testframework/testcontainers/IgniteContainer.java:
##########
@@ -439,13 +467,71 @@ private static File testClassesJar() throws IOException {
}
}
+ /**
+ * 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.
+ *
+ * @param clsPath Resource path of the top-level class (package separator
replaced with '/', ending in {@code .class}).
+ * @param nestedPrefix Package-based prefix of the nested classes, e.g.
{@code org/apache/foo/Simple$}.
+ * @return Resource names of the class and its nested classes.
+ */
+ private static Collection<String> classResources(String clsPath, String
nestedPrefix) throws IOException {
+ URL url = IgniteContainer.class.getClassLoader().getResource(clsPath);
+
+ if (url == null)
+ throw new IOException("Class not found on classpath: " + clsPath);
+
+ List<String> res = new ArrayList<>();
+
+ res.add(clsPath);
Review Comment:
classResources() returns the top-level class twice on the file: path, so
building the test-classes jar always fails.
Line 486 unconditionally seeds the list with res.add(clsPath), and then the
file: branch's filter on line 496 explicitly matches name.equals(simple +
".class") and re-adds the very same resource as pkg + f.getName().
testClassesJar() calls out.putNextEntry(new JarEntry(resName)) per element, and
ZipOutputStream.putNextEntry throws ZipException: duplicate entry on the second
occurrence.
This is the default execution path: all four classes in testClasses() live
in modules/compatibility/src/test/java, so under surefire they resolve from
target/test-classes (protocol file). Since testClassesJar() is called from the
constructor (line 200), IgniteRebalanceOnUpgradeTest fails at new
IgniteContainer(...) before any container starts.
##########
modules/compatibility/src/test/java/org/apache/ignite/compatibility/testframework/testcontainers/IgniteContainer.java:
##########
@@ -439,13 +467,71 @@ private static File testClassesJar() throws IOException {
}
}
+ /**
+ * 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.
+ *
+ * @param clsPath Resource path of the top-level class (package separator
replaced with '/', ending in {@code .class}).
+ * @param nestedPrefix Package-based prefix of the nested classes, e.g.
{@code org/apache/foo/Simple$}.
+ * @return Resource names of the class and its nested classes.
+ */
+ private static Collection<String> classResources(String clsPath, String
nestedPrefix) throws IOException {
Review Comment:
Two small redundancies in classResources():
- nestedPrefix is a parameter but only the jar: branch uses it — the file:
branch recomputes pkg/simple from clsPath itself. It could just be derived
inside the method and the parameter dropped.
- The method resolves getResource(clsPath) and the caller then resolves
getResource(resName) again for the same top-level class.
--
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]
