Add YugabyteDB module#4372
Conversation
a4b6e20 to
45273e7
Compare
|
Hi, |
eddumelendez
left a comment
There was a problem hiding this comment.
Thanks for the PR @srinivasa-vasu ! LGTM overall but WDYT about having YugabyteContainer only and provide something like withAPI(YugabyteAPI.YSQL), withAPI(YugabyteAPI.YCQL)?
@eddumelendez - Thanks for reviewing this. The APIs' are different, one is fully relational, and the other is semi and Cassandra compatible. Will end up with a lot of conditional blocks. Having it separate may be a cleaner abstraction. |
|
thanks for addressing the comments so fast! We will take a look at the changes |
eddumelendez
left a comment
There was a problem hiding this comment.
leaving additional comments and still trying to figure it out how to handle one container class for both APIs.
| @Override | ||
| protected void containerIsStarted(InspectContainerResponse containerInfo) { | ||
| if (initScript != null) { | ||
| ScriptUtils.runInitScript(new YugabyteDBYCQLDelegate(getSessionBuilder()), initScript); | ||
| } | ||
| } |
There was a problem hiding this comment.
initScript is a very convenient method but there are existing tools such as liquibase-cassandra with this purpose and execInContainer() would help to perform similar operations. Doing so, we can remove YugabyteDBYCQLDelegate. WDYT @kiview ?
There was a problem hiding this comment.
I've decoupled the Cassandra client library dependency but kept the API as is. Removed Cassandra dependency from YugabyteDBYCQLDelegate. This API may be useful for standalone services. Let me know WDYT?
ab4aa7c to
47a6e03
Compare
|
@srinivasa-vasu can you please add yugabyte to
|
a91af2d to
bbaa4aa
Compare
|
@eddumelendez - anything pending from my side? |
435c987 to
c7d68ea
Compare
eddumelendez
left a comment
There was a problem hiding this comment.
@srinivasa-vasu sorry for the delay. I have left a couple of comments about using assertj instead.
| /** | ||
| * @param initScript path of the initialization script file | ||
| * @return {@link YugabyteDBYCQLContainer} instance | ||
| */ | ||
| public YugabyteDBYCQLContainer withInitScript(String initScript) { | ||
| this.initScript = initScript; | ||
| return this; | ||
| } |
There was a problem hiding this comment.
I think this should not be supported because Testcontainers responsibility is about taking care of the service initialization. There are other tools which can help about keyspace and data management such as liquibase-cassandra. Also, that way we are avoiding being tight to a Cassandra library version. @kiview WDYT?
There was a problem hiding this comment.
I agree with @eddumelendez. Adding such an API increases complexity more than necessary.
There was a problem hiding this comment.
I've decoupled the Cassandra client library dependency but kept the API as is. This API may be useful for standalone services. Let me know WDYT?
c7d68ea to
dba2a50
Compare
eddumelendez
left a comment
There was a problem hiding this comment.
Just some suggestions in order to pass checks. Make sure to perform ./gradlew checkstyleMain checkstyleTest spotlessApply before pushing changes to make sure all checks are ok.
| */ | ||
| @RequiredArgsConstructor | ||
| @Slf4j | ||
| public final class YugabyteDBYCQLWaitStrategy extends AbstractWaitStrategy { |
There was a problem hiding this comment.
this indeed now looks very convenient. I wonder if can do something similar to our existing CassandraContainer and rid of the cassandra driver dependency at all, at least for now.
@srinivasa-vasu to give you a little more context, on sql there are some issues due to how each database handles the scripts. We recommend using tools such as liquibase or flyway in order to execute scripts that are specialized for that kind of jobs. TBH, not sure if we will have similar issues with CQL and for that reason we will try to avoid it. IMO, we should avoid it and promote best practices around database management in general. No need to take action yet on this. @kiview WDYT?
dba2a50 to
0af5d61
Compare
Signed-off-by: Srinivasa Vasu <srinivasan.surprise@gmail.com>
Signed-off-by: Srinivasa Vasu <srinivasan.surprise@gmail.com>
Signed-off-by: Srinivasa Vasu <srinivasan.surprise@gmail.com>
Signed-off-by: Srinivasa Vasu <srinivasan.surprise@gmail.com>
Signed-off-by: Srinivasa Vasu <srinivasan.surprise@gmail.com>
Signed-off-by: Srinivasa Vasu <srinivasan.surprise@gmail.com>
Signed-off-by: Srinivasa Vasu <srinivasan.surprise@gmail.com>
Signed-off-by: Srinivasa Vasu <srinivasan.surprise@gmail.com>
Signed-off-by: Srinivasa Vasu <srinivasan.surprise@gmail.com>
Signed-off-by: Srinivasa Vasu <srinivasan.surprise@gmail.com>
Signed-off-by: Srinivasa Vasu <srinivasan.surprise@gmail.com>
Signed-off-by: Srinivasa Vasu <srinivasan.surprise@gmail.com>
…acking Signed-off-by: Srinivasa Vasu <srinivasan.surprise@gmail.com>
Signed-off-by: Srinivasa Vasu <srinivasan.surprise@gmail.com>
Signed-off-by: Srinivasa Vasu <srinivasan.surprise@gmail.com>
Signed-off-by: Srinivasa Vasu <srinivasan.surprise@gmail.com>
Signed-off-by: Srinivasa Vasu <srinivasan.surprise@gmail.com>
0af5d61 to
1bc628e
Compare
|
thank you so much for your contribution @srinivasa-vasu ! This is now in |
|
Thanks @eddumelendez |
Every item on this list will require judgement by the Testcontainers core maintainers. Exceptions will sometimes be possible; items with should are more likely to be negotiable than those items with must.
Default docker image
Module dependencies
API (e.g. MyModuleContainer class)
Documentation
Every module MUST have a dedicated documentation page containing: