Skip to content

feat(rust): update CloudFetchConfig with new fields and corrected defaults#355

Open
eric-wang-1990 wants to merge 4 commits intomainfrom
stack/pr-phase1-foundation
Open

feat(rust): update CloudFetchConfig with new fields and corrected defaults#355
eric-wang-1990 wants to merge 4 commits intomainfrom
stack/pr-phase1-foundation

Conversation

@eric-wang-1990
Copy link
Collaborator

@eric-wang-1990 eric-wang-1990 commented Mar 18, 2026

🥞 Stacked PR

Use this link to review incremental changes.


What's Changed

Please fill in a description of the changes here.

This contains breaking changes.

Closes #NNN.

@eric-wang-1990 eric-wang-1990 changed the title Update CloudFetchConfig with new fields and corrected defaults\n\nTask ID: task-1.1-update-cloudfetch-config feat(rust): update CloudFetchConfig with new fields and corrected defaults Mar 18, 2026
@eric-wang-1990 eric-wang-1990 force-pushed the stack/pr-phase1-foundation branch from 7eedea9 to ba0f0e7 Compare March 18, 2026 07:28
@eric-wang-1990
Copy link
Collaborator Author

test_cloudfetch_url_expiry_triggers_refetch_and_continues is failing CI because it tests 403 reactive refetch logic that doesn't exist yet in this branch — the pre-flight is_expired() check passes (mock sets expiration 1hr in the future), so the driver downloads the /expired URL, gets a 403, and propagates it to the consumer.

Since the actual 403 handling presumably lands in phase2 or phase3, this test should be #[ignore] in this branch (like the other two tests in the file already are) and un-ignored when the implementation lands. Otherwise it blocks CI and makes it hard to distinguish expected vs real failures.


This comment was generated with GitHub MCP.

@eric-wang-1990
Copy link
Collaborator Author

[Medium] is_multiple_of() uses a recently-stabilized API

tests/e2e_cloudfetch.rs uses total_batches.is_multiple_of(50). This was stabilized in Rust 1.86 (released March 2025). If the MSRV or CI toolchain is older than 1.86, this will fail to compile on stable.

Replace with:

if total_batches % 50 == 0 {

This comment was generated with GitHub MCP.

@eric-wang-1990
Copy link
Collaborator Author

[Medium] Fire-and-forget initialize() silently fails — consumer blocks forever

streaming_provider.rs spawns initialize() as a background task but provides no signal back if link fetching fails. If fetch_links() errors during init, the consumer calls wait_for_chunk(0) and blocks indefinitely with no error surfaced.

Consider using a tokio::sync::oneshot channel to propagate initialization failure, or check for the error in next_batch() before waiting:

let (init_tx, init_rx) = tokio::sync::oneshot::channel::<Result<()>>();
// ... spawn init task, send Ok/Err via init_tx
// In next_batch(): check init_rx first

This comment was generated with GitHub MCP.

@eric-wang-1990
Copy link
Collaborator Author

[Medium] Timeout in wait_for_chunk loops without a termination condition

The timeout branch in wait_for_chunk logs and continues the loop, but there is no maximum retry count. If a download is permanently stuck (worker died without sending), this loops forever logging timeouts.

Add a max timeout count:

let mut timeout_count = 0u32;
// ...
_ = tokio::time::sleep(timeout) => {
    timeout_count += 1;
    if timeout_count >= 3 {
        return Err(DatabricksErrorHelper::io()
            .message(format!("Chunk {} not ready after {} timeouts", chunk_index, timeout_count)));
    }
    debug!("Timeout waiting for chunk {} ({}/3)", chunk_index, timeout_count);
}

This comment was generated with GitHub MCP.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant