Integration test fixes and ttl/writetime auto flag default changes to false #235

arvy · 2023-12-15T01:30:20Z

In two places in environment.sh we do a check to see if a container is up and running, but we user docker ps -a, which lists all containers (if those that are not running). I encountered this issue while trying to run the integration tests locally:

++ _testDockerCDM_Directory
++ docker exec cdm-sit-cdm ls /local/cassandra-data-migrator.jar
++ rtn=1
++ '[' 1 -eq 0 ']'
++ echo no
+ '[' no '!=' yes ']'
+ _createDockerCDM_Directory
+ docker exec cdm-sit-cdm mkdir -p /local
Error response from daemon: Container 2ba95ecae35c69594c57fdfe41d872df4b1dac98a73cded529c47087c6617466 is not running
make: *** [env_setup] Error 1

Checklist:

Automated Tests added/updated
Documentation added/updated
CLA Signed: DataStax CLA

msmygit · 2023-12-15T13:08:08Z

SIT/test.sh

-  _info "Moving ${testDir}/output/$(basename ${testDir})/*.err TO ${testDir}/output"
-  mv ${testDir}/output/$(basename ${testDir})/*.err ${testDir}/output
+  docker cp ${DOCKER_CDM}:/${testDir} ${testDir}/output/
+  _info "Moving ${testDir}/output/$(basename ${testDir})/output/*.out TO ${testDir}/output"


Suggested change

_info "Moving ${testDir}/output/$(basename ${testDir})/output/*.out TO ${testDir}/output"

_info "Moving ${testDir}/output/$(basename ${testDir})/output/*.out TO ${testDir}/output/"

msmygit · 2023-12-15T13:08:48Z

src/main/java/com/datastax/cdm/properties/KnownProperties.java

@@ -85,10 +85,10 @@ public enum PropertyType {
        required.add(ORIGIN_KEYSPACE_TABLE);
           types.put(ORIGIN_TTL_NAMES, PropertyType.STRING_LIST);
           types.put(ORIGIN_TTL_AUTO, PropertyType.BOOLEAN);
-        defaults.put(ORIGIN_TTL_AUTO, "true");


Curious, does these changes fixes any integration tests?

msmygit · 2023-12-15T13:41:35Z

SIT/environment.sh

@@ -81,7 +81,7 @@ _testDockerNetwork() {
 }

 _testDockerCassandra() {
-  dockerPs=$(docker ps -a | awk '{if ($NF == "'${DOCKER_CASS}'") {print "yes"}}')
+  dockerPs=$(docker ps | awk '{if ($NF == "'${DOCKER_CASS}'") {print "yes"}}')


Now with this change in place, I'm running into this error,

INFO Starting Docker container cdm-sit-cass docker: Error response from daemon: Conflict. The container name "/cdm-sit-cass" is already in use by container "37ef23ba475bc76e1e80fec2b8c80edbc336a5c1149261912fbfd2571f1b0ad0". You have to remove (or rename) that container to be able to reuse that name. See 'docker run --help'.

Could you please showcase how you originally encountered the error that you have in the PR description and provide reproduction steps? TY!

Try this - kill your ITs in the middle of the run after the docker containers come up and then restart your docker engine. This will stop the running containers, but they will remain there and continue to get returned in the docker ps -a output.

msmygit · 2023-12-15T13:41:43Z

SIT/environment.sh

@@ -81,7 +81,7 @@ _testDockerNetwork() {
 }

 _testDockerCassandra() {
-  dockerPs=$(docker ps -a | awk '{if ($NF == "'${DOCKER_CASS}'") {print "yes"}}')
+  dockerPs=$(docker ps | awk '{if ($NF == "'${DOCKER_CASS}'") {print "yes"}}')


Now with this change in place, I'm running into this error,

INFO Starting Docker container cdm-sit-cass docker: Error response from daemon: Conflict. The container name "/cdm-sit-cass" is already in use by container "37ef23ba475bc76e1e80fec2b8c80edbc336a5c1149261912fbfd2571f1b0ad0". You have to remove (or rename) that container to be able to reuse that name. See 'docker run --help'.

Could you please showcase how you originally encountered the error that you have in the PR description and provide reproduction steps? TY!

msmygit · 2023-12-15T15:36:54Z

SIT/test.sh

-  mv ${testDir}/output/$(basename ${testDir})/*.err ${testDir}/output
+  docker cp ${DOCKER_CDM}:/${testDir} ${testDir}/output/
+  _info "Moving ${testDir}/output/$(basename ${testDir})/output/*.out TO ${testDir}/output"
+  mv ${testDir}/output/$(basename ${testDir})/output/*.out ${testDir}/output/


This line for example is resolving as,

INFO Moving smoke/05_reserved_keyword/output/05_reserved_keyword/*.err TO smoke/05_reserved_keyword/output

where, smoke/05_reserved_keyword/output/05_reserved_keyword directory doesn't exist today

arvy · 2023-12-15T15:41:28Z

Since the auto flag is now influencing whether the feature is enabled, I flipped the default value to false, since one of the ITs was failing with a validation error, expecting the feature to be disabled. @mieslep sais default should be true.. ..i'll revert that change and figure out how to deal with the failing IT..

msmygit · 2023-12-15T16:09:25Z

src/main/java/com/datastax/cdm/properties/KnownProperties.java

           types.put(ORIGIN_WRITETIME_NAMES, PropertyType.STRING_LIST);
           types.put(ORIGIN_WRITETIME_AUTO, PropertyType.BOOLEAN);
-        defaults.put(ORIGIN_WRITETIME_AUTO, "true");
+        defaults.put(ORIGIN_WRITETIME_AUTO, "false");


@arvy I believe you may be right here. For example, if we have a counter table, then we receive the following when it is turned on,

2023-12-15 15:45:31 ERROR [] WritetimeTTL:117 - Counter table cannot specify TTL or WriteTimestamp columns as they cannot set on write 2023-12-15 15:45:31 ERROR [] CopyJobSession:80 - Feature com.datastax.cdm.feature.WritetimeTTL is not valid. Please check the configuration.

msmygit · 2023-12-18T14:41:31Z

Marking this PR as closed as the main is now back to stable. Thank you!

msmygit · 2023-12-19T14:21:35Z

@all-contributors please add @arvy for userTesting doc

allcontributors · 2023-12-19T14:21:45Z

@msmygit

I've put up a pull request to add @arvy! 🎉

arvy requested a review from a team as a code owner December 15, 2023 01:30

arvy requested review from jeromatron, Ankitp1342, msmygit and faizalrub-datastax December 15, 2023 01:30

arvydasj added 2 commits December 15, 2023 01:50

Intgration test fixes

81869b2

Changes writetime/ttl auto flag default to false

f57479f

arvy force-pushed the fixes-container-check-in-integrationtests branch from e4ed804 to f57479f Compare December 15, 2023 09:51

arvy changed the title ~~Fixes container check in integration tests.~~ Integration test fixes and ttl/writetime auto flag default changes to false Dec 15, 2023

msmygit requested review from pravinbhat and mieslep December 15, 2023 13:05

msmygit reviewed Dec 15, 2023

View reviewed changes

msmygit closed this Dec 18, 2023

allcontributors bot mentioned this pull request Dec 19, 2023

docs: add arvy as a contributor for userTesting, and doc #239

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Integration test fixes and ttl/writetime auto flag default changes to false #235

Integration test fixes and ttl/writetime auto flag default changes to false #235

arvy commented Dec 15, 2023

msmygit Dec 15, 2023

msmygit Dec 15, 2023

msmygit Dec 15, 2023

arvy Dec 15, 2023

msmygit Dec 15, 2023

msmygit Dec 15, 2023

arvy commented Dec 15, 2023

msmygit Dec 15, 2023

msmygit commented Dec 18, 2023

msmygit commented Dec 19, 2023

allcontributors bot commented Dec 19, 2023

	_info "Moving ${testDir}/output/$(basename ${testDir})/output/*.out TO ${testDir}/output"
	_info "Moving ${testDir}/output/$(basename ${testDir})/output/*.out TO ${testDir}/output/"

Integration test fixes and ttl/writetime auto flag default changes to false #235

Integration test fixes and ttl/writetime auto flag default changes to false #235

Conversation

arvy commented Dec 15, 2023

msmygit Dec 15, 2023

Choose a reason for hiding this comment

msmygit Dec 15, 2023

Choose a reason for hiding this comment

msmygit Dec 15, 2023

Choose a reason for hiding this comment

arvy Dec 15, 2023

Choose a reason for hiding this comment

msmygit Dec 15, 2023

Choose a reason for hiding this comment

msmygit Dec 15, 2023

Choose a reason for hiding this comment

arvy commented Dec 15, 2023

msmygit Dec 15, 2023

Choose a reason for hiding this comment

msmygit commented Dec 18, 2023

msmygit commented Dec 19, 2023

allcontributors bot commented Dec 19, 2023