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.

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

Add Timeplus module #8779

Merged
merged 11 commits into from
Aug 14, 2024
Merged

Conversation

lizhou1111
Copy link
Contributor

@lizhou1111 lizhou1111 commented Jun 14, 2024

Timeplus is a simple, powerful, and cost-efficient stream processing platform. It's available in different ways:

Timeplus Proton: the core engine open-sourced at GitHub under Apache 2.0 License. It is a fast and lightweight streaming SQL engine.
Timeplus Enterprise: the production-ready commercial product, with 2 deployment options:
Cloud: a fully-managed unified platform for streaming and historical data processing.
Self-hosted: same feature sets as Timeplus Cloud, able to run and scale everywhere, from edge to your data center. Customize according to your configuration needs.
Timeplus provides powerful end-to-end capabilities to help data teams process streaming and historical data quickly and intuitively, accessible for organizations of all sizes and industries. It enables data engineers and platform engineers to unlock streaming data value using SQL.

@lizhou1111 lizhou1111 requested a review from a team as a code owner June 14, 2024 09:50
@lizhou1111 lizhou1111 marked this pull request as draft June 14, 2024 09:50
@lizhou1111 lizhou1111 force-pushed the add_timeplus_container branch 2 times, most recently from 1e57c4f to 7f71364 Compare July 4, 2024 02:56
@lizhou1111 lizhou1111 marked this pull request as ready for review July 4, 2024 02:57
@lizhou1111 lizhou1111 force-pushed the add_timeplus_container branch from 56d393a to ece9945 Compare July 4, 2024 03:04
Comment on lines 18 to 20
private static final DockerImageName DEFAULT_IMAGE_NAME = DockerImageName.parse("timeplus/timeplusd:2.2.10");

private static final DockerImageName TIMEPLUS_IMAGE_NAME = DockerImageName.parse("timeplus/timeplusd:2.3.3");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
private static final DockerImageName DEFAULT_IMAGE_NAME = DockerImageName.parse("timeplus/timeplusd:2.2.10");
private static final DockerImageName TIMEPLUS_IMAGE_NAME = DockerImageName.parse("timeplus/timeplusd:2.3.3");
private static final DockerImageName DEFAULT_IMAGE_NAME = DockerImageName.parse("timeplus/timeplusd");

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Comment on lines 22 to 26
@Deprecated
public static final String IMAGE = DEFAULT_IMAGE_NAME.getUnversionedPart();

@Deprecated
public static final String DEFAULT_TAG = "latest";
Copy link
Member

Choose a reason for hiding this comment

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

delete deprecated constants

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok i will do it

Comment on lines 28 to 30
public static final Integer HTTP_PORT = 3128;

public static final Integer NATIVE_PORT = 8463;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
public static final Integer HTTP_PORT = 3128;
public static final Integer NATIVE_PORT = 8463;
private static final Integer HTTP_PORT = 3128;
private static final Integer NATIVE_PORT = 8463;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done

Comment on lines 48 to 55
/**
* @deprecated use {@link #TimeplusContainer(DockerImageName)} instead
*/
@Deprecated
public TimeplusContainer() {
this(DEFAULT_IMAGE_NAME.withTag(DEFAULT_TAG));
}

Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/**
* @deprecated use {@link #TimeplusContainer(DockerImageName)} instead
*/
@Deprecated
public TimeplusContainer() {
this(DEFAULT_IMAGE_NAME.withTag(DEFAULT_TAG));
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

deleted

Comment on lines 66 to 70
this.waitStrategy =
new HttpWaitStrategy()
.forStatusCode(200)
.forResponsePredicate("Ok."::equals)
.withStartupTimeout(Duration.ofMinutes(1));
Copy link
Member

Choose a reason for hiding this comment

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

use waitingFor(Wait.forLog...)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

HttpWaitStrategy seems don't have waitingFor function, did you mean waitUntilReady? Could you offer more info?

Copy link
Member

Choose a reason for hiding this comment

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

sorry, it is Wait.forHttp

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No need, thank you for telling me this, i'm not familiar with testconatiner code, so please feel free to comment, i will fix them.

@lizhou1111 lizhou1111 force-pushed the add_timeplus_container branch from fd8678c to ece9945 Compare July 5, 2024 03:06

private static final String NAME = "timeplus";

private static final DockerImageName TIMEPLUS_IMAGE_NAME = DockerImageName.parse("timeplus/timeplusd:2.3.3");

Choose a reason for hiding this comment

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

please use timeplus/timeplusd:develop

Copy link

Choose a reason for hiding this comment

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

no we shouldn't use latest image, use stable version instead

Copy link

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

Let's removed. New modules do not provide an specific tag. It should be provided by the consumer.

Suggested change
private static final DockerImageName TIMEPLUS_IMAGE_NAME = DockerImageName.parse("timeplus/timeplusd:2.3.3");

JDBC_URL_PREFIX +
getHost() +
":" +
getMappedPort(HTTP_PORT) +

Choose a reason for hiding this comment

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

use NATIVE_PORT instead of HTTP_PORT

Comment on lines 45 to 50
this.waitStrategy =
Wait
.forHttp("/")
.forStatusCode(200)
.forResponsePredicate("Ok."::equals)
.withStartupTimeout(Duration.ofMinutes(1));

Choose a reason for hiding this comment

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

Suggested change
this.waitStrategy =
Wait
.forHttp("/")
.forStatusCode(200)
.forResponsePredicate("Ok."::equals)
.withStartupTimeout(Duration.ofMinutes(1));
this.waitStrategy =
Wait.forHttp("/timeplusd/v1/ping").forStatusCode(200).withStartupTimeout(Duration.ofMinutes(1));

@eddumelendez
Copy link
Member

Please, read the Pull request template and the instructions for new modules.

@lizhou1111 lizhou1111 requested a review from eddumelendez July 16, 2024 02:16
@lizhou1111 lizhou1111 force-pushed the add_timeplus_container branch from baf1ce7 to e6cce0f Compare July 16, 2024 02:53
@lizhou1111
Copy link
Contributor Author

later, will add the doc related


private static final String NAME = "timeplus";

private static final DockerImageName TIMEPLUS_IMAGE_NAME = DockerImageName.parse("timeplus/timeplusd:2.3.3");
Copy link
Member

Choose a reason for hiding this comment

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

Let's removed. New modules do not provide an specific tag. It should be provided by the consumer.

Suggested change
private static final DockerImageName TIMEPLUS_IMAGE_NAME = DockerImageName.parse("timeplus/timeplusd:2.3.3");

Comment on lines 47 to 48
this.waitStrategy =
Wait.forHttp("/timeplusd/v1/ping").forStatusCode(200).withStartupTimeout(Duration.ofMinutes(1));
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
this.waitStrategy =
Wait.forHttp("/timeplusd/v1/ping").forStatusCode(200).withStartupTimeout(Duration.ofMinutes(1));
waitFor(
Wait.forHttp("/timeplusd/v1/ping").forStatusCode(200).withStartupTimeout(Duration.ofMinutes(1)));

Comment on lines 53 to 55
withEnv("CLICKHOUSE_DB", this.databaseName);
withEnv("CLICKHOUSE_USER", this.username);
withEnv("CLICKHOUSE_PASSWORD", this.password);
Copy link
Member

Choose a reason for hiding this comment

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

are those env vars right?

@lizhou1111 lizhou1111 force-pushed the add_timeplus_container branch from 56dc6bb to 00536d1 Compare July 17, 2024 08:48
@@ -0,0 +1,24 @@
# timeplus Module
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
# timeplus Module
# Timeplus Module


testImplementation project(':jdbc-test')
testRuntimeOnly 'com.timeplus:timeplus-native-jdbc:2.0.1'
testImplementation 'org.assertj:assertj-core:3.25.3'
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
testImplementation 'org.assertj:assertj-core:3.25.3'
testImplementation 'org.assertj:assertj-core:3.26.3'


static final String DOCKER_IMAGE_NAME = "timeplus/timeplusd";

private static final DockerImageName TIMEPLUS_IMAGE_NAME = DockerImageName.parse("timeplus/timeplusd");
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
private static final DockerImageName TIMEPLUS_IMAGE_NAME = DockerImageName.parse("timeplus/timeplusd");
private static final DockerImageName TIMEPLUS_IMAGE_NAME = DockerImageName.parse(DOCKER_IMAGE_NAME);


@Override
public String getUsername() {
return username;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return username;
return this.username;


@Override
public String getPassword() {
return password;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return password;
return this.password;


@Override
public String getDatabaseName() {
return databaseName;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return databaseName;
return this.databaseName;


import static org.assertj.core.api.Assertions.assertThat;

@RunWith(Parameterized.class)
Copy link
Member

Choose a reason for hiding this comment

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

It can be run without being Parameterized

@eddumelendez eddumelendez changed the title add Timeplus container for test Add Timeplus module Jul 29, 2024
@Jasmine-ge Jasmine-ge force-pushed the add_timeplus_container branch from db409c9 to 062c80b Compare August 13, 2024 07:51
@Jasmine-ge Jasmine-ge force-pushed the add_timeplus_container branch from 062c80b to 69a6974 Compare August 13, 2024 08:01
@Jasmine-ge
Copy link
Contributor

@eddumelendez i have revised about these comments, could you please take a look again

@eddumelendez
Copy link
Member

Hi @Jasmine-ge, tests are failing. Can you please take a look?

@eddumelendez
Copy link
Member

Small thing: Please, add modules/databases/timeplus.md here.

@Jasmine-ge
Copy link
Contributor

@eddumelendez Got it. Thank you very much for your help, I'm working on it.

@Jasmine-ge Jasmine-ge force-pushed the add_timeplus_container branch from b29c4a9 to 6d9d81a Compare August 14, 2024 06:34
@Jasmine-ge Jasmine-ge force-pushed the add_timeplus_container branch from 6d9d81a to 858b0ca Compare August 14, 2024 06:37
@Jasmine-ge
Copy link
Contributor

@eddumelendez I've fixed the problem that causes the failure test and added the information. Also I‘ve changed our database version to a stable one. Could you please take a look? I'm deeply grateful for your time.

@eddumelendez eddumelendez added this to the next milestone Aug 14, 2024
@eddumelendez eddumelendez merged commit 0f9956a into testcontainers:main Aug 14, 2024
102 checks passed
@eddumelendez
Copy link
Member

Thanks for your contribution @lizhou1111, @Jasmine-ge ! The new module will be available in the next release.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
github_actions Pull requests that update Github_actions code type/docs type/new module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants