IGNITE-29026 Introduce extension points in compatibility testcontainers - #13540
IGNITE-29026 Introduce extension points in compatibility testcontainers#13540wernerdv wants to merge 1 commit into
Conversation
f593694 to
c715e55
Compare
wernerdv
changed the title
wernerdv
changed the title
c715e55 to
4049e94
Compare
|
|
||
| List<String> res = new ArrayList<>(); | ||
|
|
||
| res.add(clsPath); |
There was a problem hiding this 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.
| this.hostname = hostname; | ||
| this.consistentId = consistentId; | ||
| workDirPath = WORK_DIR_PATH + "/" + hostname; | ||
| rootDir = rootDirPath(); |
There was a problem hiding this 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.
|
|
||
| /** 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; |
There was a problem hiding this 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, File>) or making it instance-level.
| } | ||
|
|
||
| /** Builds the node containers. */ | ||
| protected void initContainers() throws Exception { |
There was a problem hiding this 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.
| * @param idx Node index. | ||
| * @return The node container. | ||
| */ | ||
| protected IgniteContainer container(String imageName, Network net, List<String> consistentIds, int idx) throws Exception { |
There was a problem hiding this comment.
container(String imageName, Network net, List 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.
| * @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 { |
There was a problem hiding this 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.
| 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()); | ||
| } |
There was a problem hiding this comment.
Can we keep net initialization at field declaration? It does not depend on constructor arguments
3 participants
Thank you for submitting the pull request to the Apache Ignite.
In order to streamline the review of the contribution
we ask you to ensure the following steps have been taken:
The Contribution Checklist
The description explains WHAT and WHY was made instead of HOW.
The following pattern must be used:
IGNITE-XXXX Change summarywhereXXXX- number of JIRA issue.(see the Maintainers list)
the
green visaattached to the JIRA ticket (see tabPR Checkat TC.Bot - Instance 1 or TC.Bot - Instance 2)Notes
If you need any help, please email dev@ignite.apache.org or ask anу advice on http://asf.slack.com #ignite channel.