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

[Improvement] [clickhouse-e2e] Enhance ClickHouse E2E testing to trigger multiple checkpoints #5476

Merged
merged 2 commits into from
Sep 15, 2023
Merged

[Improvement] [clickhouse-e2e] Enhance ClickHouse E2E testing to trigger multiple checkpoints #5476

merged 2 commits into from
Sep 15, 2023

Conversation

Carl-Zhou-CN
Copy link
Member

Enhance ClickHouse E2E testing to trigger multiple checkpoints

#5468

Purpose of this pull request

Check list

ruanwenjun
ruanwenjun reviewed Sep 14, 2023
View reviewed changes
...use-e2e/src/test/java/org/apache/seatunnel/connectors/seatunnel/clickhouse/ClickhouseIT.java Outdated
@@ -76,7 +76,7 @@

public class ClickhouseIT extends TestSuiteBase implements TestResource {
private static final Logger LOG = LoggerFactory.getLogger(ClickhouseIT.class);
private static final String CLICKHOUSE_DOCKER_IMAGE = "yandex/clickhouse-server:22.1.3.7";
private static final String CLICKHOUSE_DOCKER_IMAGE = "clickhouse/clickhouse-server:latest";
Copy link
Member

@ruanwenjun ruanwenjun Sep 14, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It's not a good idea to change the image to the latest version.

Copy link
Member Author

Choose a reason for hiding this comment

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

@ruanwenjun ClickhouseSinkCDCChangelogIT uses clickhouse/clickhouse-server:latest. Should we unify it to yandex/clickhouse-server:22.1.3.7

Copy link
Member

Choose a reason for hiding this comment

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

I think @ruanwenjun means don't use latest, the version it represents can be changed at any time. Just use a certain version.

ClickhouseSinkCDCChangelogIT uses clickhouse/clickhouse-server:latest. Should we unify it to yandex/clickhouse-server:22.1.3.7

Just unify use version which clickhouse/clickhouse-server:latest represents. (Seem like is 23.3.13.6)

Copy link
Member Author

Choose a reason for hiding this comment

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

Just unify use version which clickhouse/clickhouse-server:latest represents. (Seem like is 23.3.13.6)

ok，I'll replace it with that version
image

@ruanwenjun ruanwenjun changed the title [Bug] [clickhouse-e2e] Enhance ClickHouse E2E testing to trigger multiple checkpoints [Improvement] [clickhouse-e2e] Enhance ClickHouse E2E testing to trigger multiple checkpoints Sep 14, 2023
@ruanwenjun ruanwenjun added the e2e label Sep 14, 2023
ruanwenjun
ruanwenjun approved these changes Sep 15, 2023
View reviewed changes
Copy link
Member

@ruanwenjun ruanwenjun left a comment

Choose a reason for hiding this comment

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

LGTM

@ruanwenjun ruanwenjun merged commit 9d7fa11 into apache:dev Sep 15, 2023
49 checks passed
gnehil pushed a commit to gnehil/seatunnel that referenced this pull request Oct 12, 2023
@Carl-Zhou-CN
[Improvement] [clickhouse-e2e] Enhance ClickHouse E2E testing to trig…
fe85c14 
…ger multiple checkpoints (apache#5476)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ruanwenjun ruanwenjun ruanwenjun approved these changes

@Hisoka-X Hisoka-X Hisoka-X approved these changes

Assignees

@Carl-Zhou-CN Carl-Zhou-CN

Labels
approved e2e reviewed
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Carl-Zhou-CN @ruanwenjun @Hisoka-X