Skip to content

Comments

refactor: Migrate cloudflare_kv from adapter::kv to Access instead#6340

Merged
Xuanwo merged 16 commits intoapache:mainfrom
kingsword09:refactor-cloudflare-kv-to-access
Sep 9, 2025
Merged

refactor: Migrate cloudflare_kv from adapter::kv to Access instead#6340
Xuanwo merged 16 commits intoapache:mainfrom
kingsword09:refactor-cloudflare-kv-to-access

Conversation

@kingsword09
Copy link
Contributor

@kingsword09 kingsword09 commented Jun 27, 2025

Which issue does this PR close?

Part #5739

Rationale for this change

Part #5739

What changes are included in this PR?

Migrate cloudflare_kv from adapter::kv to Access instead

Are there any user-facing changes?

@kingsword09 kingsword09 requested a review from Xuanwo as a code owner June 27, 2025 07:38
@dosubot dosubot bot added the size:XL This PR changes 500-999 lines, ignoring generated files. label Jun 27, 2025
@kingsword09 kingsword09 marked this pull request as draft June 27, 2025 07:38
@dosubot dosubot bot added the releases-note/refactor The PR does a refactor on code or has a title that begins with "refactor" label Jun 27, 2025
@kingsword09
Copy link
Contributor Author

kingsword09 commented Jun 27, 2025

The free Cloudflare account seems to trigger timeouts easily, which causes test failures.

@kingsword09
Copy link
Contributor Author

kingsword09 commented Jun 27, 2025

Should key-value stores support create_dir? Many tests assume create_dir is available by default, which leads to test failures.

@kingsword09
Copy link
Contributor Author

kingsword09 commented Jun 28, 2025

The bulk/get API seems to have issues—it consistently returns a 400 error regardless of how it’s called. Therefore, I’ve switched to using individual get requests combined with metadata to perform reads.

@kingsword09
Copy link
Contributor Author

kingsword09 commented Jun 28, 2025 •
edited
Loading

Cloudflare’s kv rest api has strict limitations, and it’s very easy to hit timeouts, which often causes test failures.

image

@kingsword09 kingsword09 changed the title refactor: Migrate cloudflare_kv from adapter::typed_kv to Access instead refactor: Migrate cloudflare_kv from adapter::kv to Access instead Jun 28, 2025
@kingsword09 kingsword09 force-pushed the refactor-cloudflare-kv-to-access branch from d8c0417 to 7d526df Compare July 13, 2025 03:54
@kingsword09 kingsword09 marked this pull request as ready for review July 13, 2025 03:54
@dosubot dosubot bot added size:XXL This PR changes 1000+ lines, ignoring generated files. and removed size:XL This PR changes 500-999 lines, ignoring generated files. labels Jul 13, 2025
@kingsword09
Copy link
Contributor Author

kingsword09 commented Jul 13, 2025

Due to the limitations of the free Cloudflare account, it is not possible to run all behavior tests successfully. However, I have verified that all tests pass locally.

kingsword09
kingsword09 commented Jul 13, 2025
View reviewed changes
core/src/services/cloudflare_kv/backend.rs
Some(limit) => {
// The list limit of cloudflare_kv is limited to 10..1000.
if !(10..=1000).contains(&limit) {
1000
Copy link
Contributor Author

@kingsword09 kingsword09 Jul 13, 2025

Choose a reason for hiding this comment

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

The reason for forcibly setting the limit to 1000 here is that a list query with limit=1 will be triggered, but Cloudflare KV requires the limit to be within 10 to 1000. Therefore, I changed it to Cloudflare's default value of 1000.

Reference:

opendal/core/src/layers/complete.rs

Line 129 in 1e3b7f5

.list(path, OpList::default().with_recursive(true).with_limit(1))

Or have any other suggestions?

Xuanwo
Xuanwo approved these changes Sep 9, 2025
View reviewed changes
Copy link
Member

@Xuanwo Xuanwo left a comment

Choose a reason for hiding this comment

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

Thank you for working on this!

@dosubot dosubot bot added the lgtm This PR has been approved by a maintainer label Sep 9, 2025
@Xuanwo Xuanwo merged commit 16bc05e into apache:main Sep 9, 2025
310 checks passed
@kingsword09 kingsword09 deleted the refactor-cloudflare-kv-to-access branch September 9, 2025 02:45
@Xuanwo
Copy link
Member

Xuanwo commented Sep 9, 2025

Hi, do you have interest to take a look over this error?

https://github.com/apache/opendal/actions/runs/17574060053/job/49915435369

Cargo build completed successfully
Copying ../c/target/debug/libopendal_c.a to lib/libopendal_c.a
     Linking opendal-test-unittest
     Running opendal-test-unittest 
object.Exception@source/opendal/operator.d(39): Failed to create Operator
----------------
/opt/hostedtoolcache/dmd/2.111.0/x64/dmd2/linux/bin64/../../src/phobos/std/exception.d:521 pure @safe noreturn std.exception.bailOut!(Exception).bailOut(immutable(char)[], ulong, scope const(char)[]) [0x55c104bc21df]
/opt/hostedtoolcache/dmd/2.111.0/x64/dmd2/linux/bin64/../../src/phobos/std/exception.d:442 pure @safe bool std.exception.enforce!().enforce!(bool).enforce(bool, lazy const(char)[], immutable(char)[], ulong) [0x55c104bc215e]
source/opendal/operator.d:39 ref @trusted opendal.operator.Operator opendal.operator.Operator.__ctor(immutable(char)[], opendal.operator.OperatorOptions, bool) [0x55c104bc146d]
source/opendal/package.d:56 @safe void opendal.__unittest_L46_C8() [0x55c104bdb009]
??:? void opendal.__modtest() [0x55c104bdb9c5]
??:? int core.runtime.runModuleUnitTests().__foreachbody_L603_C5(object.ModuleInfo*) [0x55c1054e92a2]
??:? int object.ModuleInfo.opApply(scope int delegate(object.ModuleInfo*)).__lambda_L2467_C13(immutable(object.ModuleInfo*)) [0x55c1054d38bb]
??:? int rt.minfo.moduleinfos_apply(scope int delegate(immutable(object.ModuleInfo*))).__foreachbody_L582_C5(ref rt.sections_elf_shared.DSO) [0x55c1054df02f]
??:? int rt.sections_elf_shared.DSO.opApply(scope int delegate(ref rt.sections_elf_shared.DSO)) [0x55c1054df0c1]
??:? int rt.minfo.moduleinfos_apply(scope int delegate(immutable(object.ModuleInfo*))) [0x55c1054defbd]
??:? int object.ModuleInfo.opApply(scope int delegate(object.ModuleInfo*)) [0x55c1054d388d]
??:? runModuleUnitTests [0x55c1054e90d7]
??:? void rt.dmain2._d_run_main2(char[][], ulong, extern (C) int function(char[][])*).runAll() [0x55c1054dd844]
??:? void rt.dmain2._d_run_main2(char[][], ulong, extern (C) int function(char[][])*).tryExec(scope void delegate()) [0x55c1054dd7d1]
??:? _d_run_main2 [0x55c1054dd73a]
??:? _d_run_main [0x55c1054dd523]
/opt/hostedtoolcache/dmd/2.111.0/x64/dmd2/linux/bin64/../../src/druntime/import/core/internal/entrypoint.d:29 main [0x55c104bbc96d]
??:? [0x7feec202a1c9]
??:? __libc_start_main [0x7feec202a28a]
??:? _start [0x55c104bbc544]
1/1 modules FAILED unittests

@kingsword09
Copy link
Contributor Author

kingsword09 commented Sep 9, 2025

Don't know much about this D language, I'll try to see if Ai can fix it.

@kingsword09 kingsword09 mentioned this pull request Sep 9, 2025
Merged
@koushiro koushiro mentioned this pull request Oct 20, 2025
Closed
22 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Xuanwo Xuanwo Xuanwo approved these changes

Assignees

No one assigned

Labels

lgtm This PR has been approved by a maintainer releases-note/refactor The PR does a refactor on code or has a title that begins with "refactor" size:XXL This PR changes 1000+ lines, ignoring generated files.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@kingsword09 @Xuanwo