Skip to content

clickhouse(depstore): return error instead of panic in GetDependencies#8172

Open
jkowall wants to merge 1 commit intojaegertracing:mainfrom
jkowall:codex/clickhouse-depstore-no-panic
Open

clickhouse(depstore): return error instead of panic in GetDependencies#8172
jkowall wants to merge 1 commit intojaegertracing:mainfrom
jkowall:codex/clickhouse-depstore-no-panic

Conversation

@jkowall
Copy link
Contributor

@jkowall jkowall commented Mar 14, 2026

Motivation

  • The ClickHouse storage backend is selectable and its dependency reader previously used panic("not implemented"), which can crash the query service when invoked and cause a DoS. This change prevents process termination by returning an error.

Description

  • Replace the panic in (*Reader).GetDependencies with a returned error.
  • Update the unit test in internal/storage/v2/clickhouse/depstore/reader_test.go to assert a nil result and the expected error instead of expecting a panic.

Testing

  • make fmt
  • make lint
  • make test

@jkowall jkowall requested a review from a team as a code owner March 14, 2026 16:10
Copilot AI review requested due to automatic review settings March 14, 2026 16:10
Copy link
Contributor

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

This PR replaces a runtime panic in the ClickHouse dependency reader with a regular error return, and updates the unit test accordingly. This aligns the ClickHouse depstore implementation with safer, production-friendly behavior when a feature is not yet implemented.

Changes:

  • Replace panic("not implemented") in GetDependencies with an error return.
  • Update the dependency reader test to assert on the returned error and nil dependencies.

Reviewed changes

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

File Description
internal/storage/v2/clickhouse/depstore/reader.go Return a non-panicking “not implemented” error from GetDependencies.
internal/storage/v2/clickhouse/depstore/reader_test.go Update test to validate the error return instead of expecting a panic.

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

@yurishkuro
Copy link
Member

commits not signed?

@github-actions github-actions bot added the waiting-for-author PR is waiting for author to respond to maintainer's comments label Mar 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area/storage bug waiting-for-author PR is waiting for author to respond to maintainer's comments

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants