Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

DBZ-6763 Fix CI tests caused by MongoDbReplicaSet which is trying to stop not running containers almost infinitely #4947

Merged
merged 1 commit into from Oct 18, 2023
Merged

DBZ-6763 Fix CI tests caused by MongoDbReplicaSet which is trying to stop not running containers almost infinitely #4947

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
17 changes: 12 additions & 5 deletions ...ng-testcontainers/src/main/java/io/debezium/testing/testcontainers/MongoDbReplicaSet.java
View file
Open in desktop
Expand Up @@ -5,7 +5,6 @@
*/
package io.debezium.testing.testcontainers;

import static io.debezium.testing.testcontainers.MongoDbContainer.node;
import static io.debezium.testing.testcontainers.util.DockerUtils.logDockerDesktopBanner;
import static java.util.concurrent.TimeUnit.MINUTES;
import static java.util.concurrent.TimeUnit.SECONDS;
Expand All @@ -14,6 +13,7 @@

import java.net.URLEncoder;
import java.nio.charset.StandardCharsets;
import java.time.Duration;
import java.util.ArrayList;
import java.util.HashSet;
import java.util.List;
Expand Down Expand Up @@ -59,7 +59,7 @@ public class MongoDbReplicaSet implements MongoDbDeployment {
private final String rootPassword;
private final Supplier<MongoDbContainer.Builder> nodeSupplier;

private boolean started;
private boolean started = false;

public static Builder replicaSet() {
return new Builder().nodeSupplier(MongoDbContainer::node);
Expand Down Expand Up @@ -278,9 +278,11 @@ public void start() {
*/
@Override
public void stop() {
LOGGER.info("[{}] Stopping...", name);
MoreStartables.deepStopSync(members.stream());
started = false;
if (started) {
LOGGER.info("[{}] Stopping...", name);
MoreStartables.deepStopSync(members.stream());
started = false;
}
}

private void initializeReplicaSet() {
Expand Down Expand Up @@ -368,6 +370,11 @@ public List<String> getHostNames() {
.collect(Collectors.toList());
}

public MongoDbReplicaSet withStartupTimeout(Duration startupTimeout) {
Copy link
Member

@jcechace jcechace Oct 18, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The entire class is not designed as a fluent builder. I don't see a reason why a single message should behave differently. This should be in the builder. Please place it there and be careful about mixing various design patterns into single thing.

Thank you.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jcechace This just goes back to your own comment here:
https://github.com/debezium/debezium/blob/main/debezium-testing/debezium-testing-testcontainers/src/main/java/io/debezium/testing/testcontainers/testhelper/RestExtensionTestInfrastructure.java#L135-L140

The Oracle Connect REST extension PR will then use this method instead of the code linked, but I decided to add it to this bugfix PR so it can be used early before we finish that other PR. Maybe you want to log a Jira to change and refactor this part/class?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rk3rn3r There is a structure to those classes. Put it into the builder, pass it from the builder to the RS class and then over to the underlying containers in a similar fashion.

I don't see a need for Jira. The code was not refactored correctly and the PR was merged before I could review it. Please make it consistent with the rest of the class design. Thank you.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jcechace This is not how we work and I only applied a change that you suggested in the comment that I linked above, which was a good comment. The class is Startable and suggested to reflect a running container. Thus, a timeout config makes sense. Feel free to fix it that it fits your individual idea of that class or provide a Jira explaining what you want to achieve.

Copy link
Member

@jcechace jcechace Oct 19, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Jira is here https://issues.redhat.com/browse/DBZ-7054

Timeout config does make sense. I have no problem with having it (we talked about that). I am just saying that the configuration wasn't added to that class consistently with other configuration options.

members.forEach(member -> member.withStartupTimeout(startupTimeout));
return this;
}

@Override
public String toString() {
return "MongoDbReplicaSet{" +
Expand Down