Skip to content

Conversation

@komamitsu
Copy link
Contributor

@komamitsu komamitsu commented Jun 17, 2024

Description

Recent Cosmos DB integration tests are slow and unstable. We also need to manage Cosmos DB related resources for the integration tests. This PR uses the Azure Cosmos DB emulator instead using actual Cosmos DB.

Related issues and/or PRs

None

Changes made

  • In the GitHub Actions workflow:
    • Launch the Cosmos DB emulator in windows-latest image
    • Install TLS certification extracted from the emulator in the Windows VM
    • Run the integration tests using the emulator
  • Reduce the concurrency of the integration test from 3 to 1 to avoid tentative errors as much as possible

Checklist

The following is a best-effort checklist. If any items in this checklist are not applicable to this PR or are dependent on other, unmerged PRs, please still mark the checkboxes after you have read and understood each item.

  • I have commented my code, particularly in hard-to-understand areas.
  • I have updated the documentation to reflect the changes.
  • Any remaining open issues linked to this PR are documented and up-to-date (Jira, GitHub, etc.).
  • Tests (unit, integration, etc.) have been added for the changes.
  • My changes generate no new warnings.
  • Any dependent changes in other PRs have been merged and published.

Additional notes (optional)

None

Release notes

N/A

# Delete all files modified more than 3 days ago with the ".out.log" file extension located in the "/home/azureuser/.gradle/daemon"
# folder hierarchy. These files accumulate over time and can end up using a lot of disk space
run : find /home/azureuser/.gradle/daemon -name "*.out.log" -type f -mtime +3 -exec rm -vf {} +
arguments: integrationTestCosmos -PjavaVersion=${{ env.JAVA_VERSION }} -PjavaVendor=${{ env.JAVA_VENDOR }} -PintegrationTestJavaRuntimeVersion=${{ env.INT_TEST_JAVA_RUNTIME_VERSION }} -PintegrationTestJavaRuntimeVendor=${{ env.INT_TEST_JAVA_RUNTIME_VENDOR }} -Dscalardb.cosmos.uri=https://localhost:8081/ -Dscalardb.cosmos.password=C2y6yDjf5/R+ob0N8A7Cgv30VRDJIWEHLM+4QDU5DE2nQ9nDuVTqobD4b8mGGyPMbIZnqyMsEcaGQy67XIw/Jw== -Dscalardb.cosmos.database_prefix=${{ env.db_prefix }}_ -Dfile.encoding=UTF-8
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This password is a public value, so hard coded here.

-Dfile.encoding=UTF-8 is needed to address some encoding issues that occur due to the WM's default charset (Windows-1252)

options {
systemProperties(System.getProperties().findAll{it.key.toString().startsWith("scalardb")})
}
maxParallelForks = 3
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Concurrent operations seem to increase the possibility of tentative errors.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Ah, okay. How long does this integration test take? Is it shorter than before?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@brfrn169 About 26 minutes.
image

The test once passed with maxParallelForks = 3 and took 19 minutes. But the test often failed with the configuration...

Copy link
Collaborator

Choose a reason for hiding this comment

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

Okay... Thank you. I wish it had solved the slow integration test problem as well. 😅

Copy link
Contributor

Choose a reason for hiding this comment

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

Using larger runners might make parallel execution feasible. It should be way more inexpensive than Cosmos DB, so it is worth trying. Let's discuss it later.

@komamitsu komamitsu self-assigned this Jun 17, 2024
@komamitsu komamitsu added the github_actions Pull requests that update GitHub Actions code label Jun 17, 2024
@brfrn169 brfrn169 requested review from brfrn169 and removed request for jnmt June 17, 2024 16:55
Comment on lines 283 to 285
- name: Generate unique prefix using the epoch
run: |
echo "db_prefix=$(date +%s%3N)" >> $GITHUB_ENV
Copy link
Contributor

@Torch3333 Torch3333 Jun 18, 2024

Choose a reason for hiding this comment

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

I think we can remove this since multiple integration test runners are not sharing the same CosmosDB instance now; there won't be database name conflicts without a prefix.
I guess we could also remove the handling of the scalardb.cosmos.database_prefix environment variable in the integration test code.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@Torch3333 Good catch! Thank you. I'll remove db_prefix related jobs and parameters from the workflow file.

For Java code, I think we may want to test with real Cosmo DB later. The implementation handling scalardb.cosmos.database_prefix might be useful for such a case. I don't have a strong opinion on it, though. What do you think?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks.
Ok, keeping the implementation handling is fine.

Copy link
Collaborator

@brfrn169 brfrn169 Jun 18, 2024

Choose a reason for hiding this comment

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

@komamitsu @Torch3333 I think we can remove scalardb.cosmos.database_prefix. This parameter is useful only when using a shared Cosmos DB account. If we want to test with real Cosmos DB later, we can create a new dedicated Cosmos DB account.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@brfrn169 Thanks. Understood. I'll create a follow up PR for that.

Copy link
Collaborator

Choose a reason for hiding this comment

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

@komamitsu Thank you!

Copy link
Contributor

@Torch3333 Torch3333 left a comment

Choose a reason for hiding this comment

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

LGTM, thank you.

Copy link
Collaborator

@brfrn169 brfrn169 left a comment

Choose a reason for hiding this comment

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

LGTM other than what I commented. Thank you!

Copy link
Contributor

@feeblefakie feeblefakie left a comment

Choose a reason for hiding this comment

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

LGTM! Thank you!

options {
systemProperties(System.getProperties().findAll{it.key.toString().startsWith("scalardb")})
}
maxParallelForks = 3
Copy link
Contributor

Choose a reason for hiding this comment

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

Using larger runners might make parallel execution feasible. It should be way more inexpensive than Cosmos DB, so it is worth trying. Let's discuss it later.

@feeblefakie feeblefakie merged commit 6d43a36 into master Jun 18, 2024
@feeblefakie feeblefakie deleted the use-cosmos-db-emu-for-it branch June 18, 2024 02:20
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 improvement

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants