Reduce the number of concurrent DDLs from 10 to 1 for Cosmos DB in the integration tests

[komamitsu]

Description

After merging #1909, we observed the integration test often failed with the Cosmos DB emulator due to failing to create a namespace.

scalardb/integration-test/src/main/java/com/scalar/db/api/DistributedStorageCrossPartitionScanIntegrationTestBase.java

Line 165 in 6d43a36

admin.createNamespace(getNamespaceName(firstColumnType), true, options);

I noticed CosmosCrossPartitionScanIntegrationTest used the default thread number (10). So, this PR limits it to 1 as well as other tests.

Related issues and/or PRs

• Use Cosmos DB emulator for the integration tests #1909

Changes made

• Reduce the concurrency for DDL operations in CosmosCrossPartitionScanIntegrationTest from 10 to 3

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)

• I first tried to reduce the concurrency to 3. But it still failed, and I changed it to 1.
• If this change makes the integration test with Cosmos DB emulator more stable, we may be able to increase maxParallelForks to 2 or 3 later.

Release notes

N/A

[feeblefakie]

LGTM! Thank you!

[Torch3333]

LGTM, thank you.

[brfrn169]

Can we override the isParallelDdlSupported() method instead of this? Or we might be able to remove the isParallelDdlSupported() method and use getThreadNum() instead for the sake of consistency.

[komamitsu]

@brfrn169 Good point! Actually I noticed the same and created a branch to remove executeDdls() and isParallelDdlSupported() a17d246.

I'll create a PR with the change for only branches 3 and master that contain those unnecessary methods (reducing concurrency for Cosmos DB integration test is needed for all the release and support branches)

[brfrn169]

@komamitsu Also, maybe, we can override isParallelDdlSupported() (and make it return false) in CosmosMultiplePartitionKeyIntegrationTest and CosmosMultipleClusteringKeyScanIntegrationTest just in case?

[komamitsu]

we can override isParallelDdlSupported()

@brfrn169 I'll remove the method later as a separate PR since the method doesn't exist in older branches than 3, while reducing the concurrency addressed with this PR is needed in all the release and support branches (#1920 (comment)).

[brfrn169]

@komamitsu Sorry, I checked the source code again. It seems like isParallelDdlSupported() is still necessary because the thread pool is used not only for DDLs but also the test execution. So maybe, in this PR, I think we can override isParallelDdlSupported() and make it return false in CosmosCrossPartitionScanIntegrationTest, CosmosMultiplePartitionKeyIntegrationTest and CosmosMultipleClusteringKeyScanIntegrationTest. What do you think?

[komamitsu]

@brfrn169 Ah, you're right. I didn't notice the executorService was also used for DML operations. But, the older branches than branch 3 don't have execDdls() and isParallelDdlSupported(), so we need to port the methods to the old branches...?

I'm wondering what is the purpose of concurrently issuing the DML operations in the test class. If it's to reduce the integration test duration, I noticed the total duration wasn't increased with a single thread and we don't need execDdls() and isParallelDdlSupported() by leveraging on getThreadNum():

• 23m24s (https://github.com/scalar-labs/scalardb/actions/runs/9563665219/attempts/1)
• 25m9s (https://github.com/scalar-labs/scalardb/actions/runs/9563665219)

With 10 threads:

• 24m17s (https://github.com/scalar-labs/scalardb/actions/runs/9557520677)

If the concurrent DML operations are needed to see if things work fine even under multi-threads, we need to separate the concurrencies for DDL and DML by introducing execDdls() and isParallelDdlSupported().

[komamitsu]

I noticed the total duration wasn't increased with a single thread and we don't need execDdls() and isParallelDdlSupported() by leveraging on getThreadNum():

However, separating the concurrencies for DDL and DML might be always helpful for flexible tuning. Let me know if you think we should add execDdls() and isParallelDdlSupported() in all the release and supported branches.

[brfrn169]

@komamitsu

I'm wondering what is the purpose of concurrently issuing the DML operations in the test class.

Actually, I don't remember the reasons.

If it's to reduce the integration test duration, I noticed the total duration wasn't increased with a single thread and we don't need execDdls() and isParallelDdlSupported() by leveraging on getThreadNum():

If it doesn't affect the integration test duration for all the storages, we can execute the DDLs in a single thread. Could you please check that? Thanks.

[komamitsu]

@brfrn169

If it doesn't affect the integration test duration for all the storages, we can execute the DDLs in a single thread. Could you please check that? Thanks.

Oh, I didn't talked about other storages at all, but only Cosmos DB. My understanding is as follows:

• Configuring getThreadNum() affects the concurrencies of DDL and DML operations
• For the Cosmos DB integration test, making getThreadNum() return 1 seemed to mitigate the namespace failure issue and the total duration didn't increase. This is the quickest solution for the Cosmos DB integration test. (I guess the bottleneck of the test cases is Cosmos DB and increasing the client side concurrency doesn't help much)
• For other storage integration test, whether concurrent DDL reduces the duration depends on underlying database/storage. The configurable concurrent DDL should be kept as is

Okay. I can port execDdls() and isParallelDdlSupported() to the older branches than 3, and then mitigate the namespace creation failure issue by isParallelDdlSupported(). Do you want me to do that?

[brfrn169]

@komamitsu

Okay. I can port execDdls() and isParallelDdlSupported() to the older branches than 3, and then mitigate the namespace creation failure issue by isParallelDdlSupported(). Do you want me to do that?

Sounds good. Thank you!

[brfrn169]

LGTM! Thank you!