Skip to content

fix: fix thread-safe problem for file store path factory#323

Merged
lucasfang merged 5 commits into
alibaba:mainfrom
lucasfang:thread-safe
Jun 2, 2026
Merged

fix: fix thread-safe problem for file store path factory#323
lucasfang merged 5 commits into
alibaba:mainfrom
lucasfang:thread-safe

Conversation

@lucasfang
Copy link
Copy Markdown
Collaborator

@lucasfang lucasfang commented Jun 1, 2026

Purpose

N/A

This change fixes a thread-safety issue in FileStorePathFactory cache access paths.

  • Protect cache reads/writes in ToBinaryRow and GetPartitionString with std::shared_mutex.
  • Use read lock for fast-path cache hits and write lock for insertions, with double-check under write lock.
  • Update API note to reflect that GetPartitionString is now thread-safe for concurrent calls.

Tests

UT:

  • FileStorePathFactoryTest.TestConcurrentToBinaryRowAndGetPartitionString
    • Runs 8 threads with 500 iterations per thread.
    • Concurrently calls ToBinaryRow and GetPartitionString and validates deterministic outputs.

Existing UTs in the same suite still pass for non-concurrent paths.

IT:

  • N/A

API and Format

No API behavior change for external callers, and no storage format/protocol change.

Internal update only:

  • Added cache_mutex_ (std::shared_mutex) to guard in-memory caches.
  • Updated method comment for thread-safety semantics.

Documentation

No new feature introduced. No documentation update required.

Generative AI tooling

Generated-by: GitHub Copilot (GPT-5.3-Codex)

Copilot AI review requested due to automatic review settings June 1, 2026 08:04
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot encountered an error and was unable to review this pull request. You can try again by re-requesting a review.

Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.

Comment thread src/paimon/core/utils/file_store_path_factory.cpp
Comment thread src/paimon/core/utils/file_store_path_factory.cpp
Comment thread src/paimon/core/utils/file_store_path_factory.cpp
Copy link
Copy Markdown
Collaborator

@lxy-9602 lxy-9602 left a comment

Choose a reason for hiding this comment

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

+1

Copy link
Copy Markdown
Collaborator

@zjw1111 zjw1111 left a comment

Choose a reason for hiding this comment

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

+1

@lucasfang lucasfang merged commit 4b2c0c0 into alibaba:main Jun 2, 2026
9 checks passed
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.

4 participants