Skip to content

[ENH](log-service): defer Spanner init to first use - #6915

Merged
rescrv merged 6 commits into
mainfrom
rescrv/lazy-spanner-init
Apr 28, 2026
Merged

[ENH](log-service): defer Spanner init to first use#6915
rescrv merged 6 commits into
mainfrom
rescrv/lazy-spanner-init

Conversation

@rescrv

@rescrv rescrv commented Apr 15, 2026

Copy link
Copy Markdown
Contributor

Description of changes

Introduce LazySpannerClient that wraps SpannerClient behind a OnceCell,
deferring connection from boot time to first use so the log server can
start even when Spanner is temporarily unreachable.

  • Map Spanner connection and session errors to Unavailable instead of
    Internal/Unknown, enabling clients to retry transient failures.
  • Replace Status::unknown with status_from_chroma_error throughout to
    propagate proper gRPC status codes from ChromaError.
  • Extract connect_spanner, build_storage_wrappers_for_topology, and
    create_repl_factories_for_topology as free functions, removing the
    duplicated topology-setup logic.
  • Add tests verifying lazy startup, error code mapping, and that init
    failures remain retryable.

Test plan

CI

Migration plan

N/A

Observability plan

N/A

Documentation Changes

N/A

Co-authored-by: AI

@github-actions

Copy link
Copy Markdown

Reviewer Checklist

Please leverage this checklist to ensure your code review is thorough before approving

Testing, Bugs, Errors, Logs, Documentation

  • Can you think of any use case in which the code does not behave as intended? Have they been tested?
  • Can you think of any inputs or external events that could break the code? Is user input validated and safe? Have they been tested?
  • If appropriate, are there adequate property based tests?
  • If appropriate, are there adequate unit tests?
  • Should any logging, debugging, tracing information be added or removed?
  • Are error messages user-friendly?
  • Have all documentation changes needed been made?
  • Have all non-obvious changes been commented?

System Compatibility

  • Are there any potential impacts on other parts of the system or backward compatibility?
  • Does this change intersect with any items on our roadmap, and if so, is there a plan for fitting them together?

Quality

  • Is this code of a unexpectedly high quality (Readability, Modularity, Intuitiveness)

@propel-code-bot

propel-code-bot Bot commented Apr 15, 2026

Copy link
Copy Markdown
Contributor

log-service: lazy Spanner initialization with retryable error mapping

This PR changes replicated log topology initialization to defer Spanner client connection until first actual use by introducing LazySpannerClient backed by tokio::sync::OnceCell. Previously, Spanner setup happened during service startup; now LogServer can boot even if Spanner is temporarily unreachable, and connection attempts occur on demand in read/write/maintenance paths that require replicated metadata access.

It also standardizes gRPC error conversion by adding status_from_chroma_error in rust/error/src/tonic.rs and replacing many Status::unknown(...) conversions in rust/log-service/src/lib.rs. Additionally, Spanner connection/session failures are mapped to ErrorCodes::Unavailable (instead of broadly Internal), and topology factory setup logic is deduplicated via extracted helpers (connect_spanner, build_storage_wrappers_for_topology, create_repl_factories_for_topology). Tests were added/updated to verify lazy startup behavior, retryable failure semantics, and new mappings.

This summary was automatically generated by @propel-code-bot

@propel-code-bot propel-code-bot Bot 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.

No issues were found; the lazy Spanner initialization and error-mapping refactor appear sound and low risk.

Status: No Issues Found | Risk: Low

Review Details

📁 2 files reviewed | 💬 0 comments

@blacksmith-sh

This comment has been minimized.

@rescrv
rescrv requested a review from sanketkedia April 16, 2026 17:20
@rescrv
rescrv force-pushed the rescrv/lazy-spanner-init branch from dc3170f to 3413a33 Compare April 16, 2026 17:33
@blacksmith-sh

This comment has been minimized.

Comment thread rust/log-service/src/lib.rs Outdated
Status::with_metadata(code, message, metadata)
}

fn status_from_chroma_error(err: impl ChromaError + std::fmt::Display) -> Status {

@sanketkedia sanketkedia Apr 25, 2026

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.

would it be useful to instead put this into the chroma-error crate so that others can also benefit from it?

rescrv added 5 commits April 27, 2026 16:25
Introduce LazySpannerClient that wraps SpannerClient behind a OnceCell,
deferring connection from boot time to first use so the log server can
start even when Spanner is temporarily unreachable.

- Map Spanner connection and session errors to Unavailable instead of
  Internal/Unknown, enabling clients to retry transient failures.
- Replace Status::unknown with status_from_chroma_error throughout to
  propagate proper gRPC status codes from ChromaError.
- Extract connect_spanner, build_storage_wrappers_for_topology, and
  create_repl_factories_for_topology as free functions, removing the
  duplicated topology-setup logic.
- Add tests verifying lazy startup, error code mapping, and that init
  failures remain retryable.

Co-authored-by: AI
@rescrv
rescrv force-pushed the rescrv/lazy-spanner-init branch from b8a30b8 to a838839 Compare April 27, 2026 23:25
@rescrv
rescrv merged commit 00db571 into main Apr 28, 2026
118 of 123 checks passed
@rescrv
rescrv deleted the rescrv/lazy-spanner-init branch April 28, 2026 16:39
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