GH-51604: [C++][Parquet] Only link opentelemetry-cpp::sdk on arrow_reader_writer_tracing_test.cc to avoid double freeing - #51606
Conversation
…arrow_reader_writer_tracing_test.cc
|
|
|
@github-actions crossbow submit test-ubuntu-24.04-cpp-thread-sanitizer |
|
Revision: 2a3cc4c Submitted crossbow builds: ursacomputing/crossbow @ actions-429379b10c
|
|
@github-actions crossbow submit test-ubuntu-24.04-cpp-thread-sanitizer |
|
Revision: 2b7b573 Submitted crossbow builds: ursacomputing/crossbow @ actions-99c824cce4
|
There was a problem hiding this comment.
Copilot review overview
🔵 Needs a closer look
Include the OpenTelemetry API target’s include directories and compile definitions.
Review effort: Lite
Findings: None
What changed in this PR
Updates the Parquet tracing test to avoid duplicate OpenTelemetry linking and sanitizer failures.
Changes:
- Removes direct OpenTelemetry test linkage.
- Adds required include directories and compile definitions.
- Preserves tracing test configuration.
| File | Summary |
|---|---|
cpp/src/parquet/CMakeLists.txt |
Updates OpenTelemetry linkage and compilation settings; API target properties still need to be included. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
@kou Can you take a look? |
| EXTRA_INCLUDES | ||
| $<TARGET_PROPERTY:opentelemetry-cpp::trace,INTERFACE_INCLUDE_DIRECTORIES> | ||
| DEFINITIONS | ||
| $<TARGET_PROPERTY:opentelemetry-cpp::trace,INTERFACE_COMPILE_DEFINITIONS> |
There was a problem hiding this comment.
Can we use opentelemetry-cpp::sdk instead? It seems that opentelemetry-cpp::sdk just provides include path and definitions.
diff --git a/cpp/src/parquet/CMakeLists.txt b/cpp/src/parquet/CMakeLists.txt
index 59ac952652..a5519cc5a2 100644
--- a/cpp/src/parquet/CMakeLists.txt
+++ b/cpp/src/parquet/CMakeLists.txt
@@ -417,7 +417,7 @@ if(ARROW_WITH_OPENTELEMETRY AND NOT ARROW_USE_ASAN)
SOURCES
arrow/arrow_reader_writer_tracing_test.cc
EXTRA_LINK_LIBS
- ${ARROW_OPENTELEMETRY_LIBS})
+ opentelemetry-cpp::sdk)
endif()
add_parquet_test(arrow-index-test SOURCES arrow/index_test.cc)There was a problem hiding this comment.
Thanks @kou, I tested locally and it does seem that linking only opentelemetry-cpp::sdk solves indeed the ASAN/TSAN issues. I have pushed the change.
github-actions
Bot
added
awaiting changes
…ARROW_OPENTELEMETRY_LIBS or manually defining EXTRA_INCLUDES and DEFINITIONS
github-actions
Bot
added
awaiting change review
|
@github-actions crossbow submit test-ubuntu-24.04-cpp-thread-sanitizer |
raulcd
changed the title
|
Revision: 2f4ee12 Submitted crossbow builds: ursacomputing/crossbow @ actions-f90e47a28f
|
github-actions
Bot
added
awaiting changes
…writer-tracing-test depending on System vs Bundled OpenTelemetry (#51633) ### Rationale for this change After merging #51606 some debian jobs that use System opentelemetry were failing with missing symbols. ### What changes are included in this PR? Change what we link depending on bundled vs system OpenTelemetry. ### Are these changes tested? Yes all the CI jobs that the previous issue and this new issue had failing have been exercised and are successful ### Are there any user-facing changes? No ### Was AI used for this PR? In accordance to the [AI generation guidelines](https://arrow.apache.org/docs/dev/developers/overview.html#ai-generated-code), please disclose below whether and how AI was used in this PR. **PR code and description written by:** - [x] Human - [ ] AI **Reviewed before submission by:** - [x] Human - [ ] AI - [ ] Not reviewed * GitHub Issue: #51632 Authored-by: Raúl Cumplido <raulcumplido@gmail.com> Signed-off-by: Sutou Kouhei <kou@clear-code.com>
Rationale for this change
ASAN and TSAN jobs were failing due to double freeing opentelemetry globals.
What changes are included in this PR?
Avoid linking all OpenTelemetry libs and only link
opentelemetry-cpp::sdkto the test to avoid double free as OpenTelemetry is already linked in libarrow. ) opentelemetry-cpp::sdk adds the required include directories and compile definitions.Are these changes tested?
Yes on CI
Are there any user-facing changes?
No
Was AI used for this PR?
In accordance to the AI generation guidelines, please disclose below whether and how AI was used in this PR.
PR code and description written by:
Code was written by a human but I've used Claude Opus 5.5 in order to analyze the problem and brainstorm solutions.
Reviewed before submission by: