Skip to content

fix(jindo): make async reads concurrency-safe#427

Open
zjw1111 wants to merge 2 commits into
alibaba:mainfrom
zjw1111:codex/fix-jindo-async-read
Open

fix(jindo): make async reads concurrency-safe#427
zjw1111 wants to merge 2 commits into
alibaba:mainfrom
zjw1111:codex/fix-jindo-async-read

Conversation

@zjw1111

@zjw1111 zjw1111 commented Jul 15, 2026

Copy link
Copy Markdown
Collaborator

Purpose

Linked issue: N/A

JindoInputStream reused one member std::string_view across synchronous and concurrent asynchronous reads, and ignored failures returned by perform(). This could race between reads or leave a caller's completion callback unresolved.

This change:

  • gives every asynchronous read an independent result and completion state
  • completes the callback exactly once, including when perform() fails
  • keeps synchronous read results local and documents the caller-side async lifetime contract
  • adds real OSS coverage for concurrent sync/async reads and listing across an OSS page boundary
  • introduces the opt-in PAIMON_ENABLE_JINDO_TESTS switch and enables Jindo cases in the existing filesystem and write/read integration test binaries

Tests

  • cmake --build build --target paimon-fs-test -j64
  • cmake --build build --target paimon-write-and-read-inte-test -j64
  • Real OSS Jindo filesystem tests: 6 passed
  • FsType/FileSystemTest.TestFactoryCreator/1
  • FileFormatAndFileSystem/WriteAndReadInteTest.TestAppendSimple/1
  • pre-commit run --files ...
  • git diff --check

API and Format

Clarifies the existing InputStream::ReadAsync lifetime contract. No storage format or protocol changes.

Documentation

No user-facing documentation changes.

Generative AI tooling

Generated-by: OpenAI Codex (GPT-5)

Copilot AI review requested due to automatic review settings July 15, 2026 11:25

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Pull request overview

This PR hardens the Jindo filesystem InputStream async read path to be concurrency-safe by removing shared per-instance read result state and ensuring async callbacks are completed exactly once, including when the underlying async task fails to start/perform. It also adds opt-in remote OSS-backed Jindo test coverage and clarifies the ReadAsync lifetime contract in the public API.

Changes:

  • Refactor JindoInputStream::ReadAsync to use per-request state and to propagate perform() failures via the callback.
  • Remove shared std::string_view member state from JindoInputStream and keep sync read results local.
  • Add PAIMON_ENABLE_JINDO_TESTS CMake option and gate OSS-backed Jindo tests/integration test parameterization behind it.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
test/inte/write_and_read_inte_test.cpp Conditionally enables "jindo" test matrix entries when PAIMON_ENABLE_JINDO_TESTS is set.
test/inte/CMakeLists.txt Adds PAIMON_ENABLE_JINDO_TESTS compile definition to the write/read integration test target when enabled.
src/paimon/fs/jindo/jindo_file_system.h Removes shared std::string_view member from JindoInputStream.
src/paimon/fs/jindo/jindo_file_system.cpp Implements per-async-read state + callback completion-on-perform-failure for concurrency safety.
src/paimon/fs/jindo/jindo_file_system_test.cpp Adds OSS-backed tests for listing across page boundaries and concurrent sync/async reads.
src/paimon/common/fs/file_system_test.cpp Conditionally includes "jindo" in filesystem unit test parameters when PAIMON_ENABLE_JINDO_TESTS is set.
src/paimon/CMakeLists.txt Gates Jindo filesystem test sources/link libs and test compile definitions behind PAIMON_ENABLE_JINDO_TESTS.
include/paimon/fs/file_system.h Clarifies InputStream::ReadAsync caller lifetime requirements in the public API doc.
CMakeLists.txt Introduces PAIMON_ENABLE_JINDO_TESTS option and enforces option dependencies.
cmake_modules/DefineOptions.cmake Exposes PAIMON_ENABLE_JINDO_TESTS via the project’s option definition helpers.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread src/paimon/fs/jindo/jindo_file_system.cpp
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.

2 participants