Alexey Serbin has posted comments on this change. ( http://gerrit.cloudera.org:8080/22924 )
Change subject: IMPALA-13235: Update the build toolchain to include the latest OpenTelemetry C++ sdk. ...................................................................... Patch Set 8: (9 comments) http://gerrit.cloudera.org:8080/#/c/22924/8//COMMIT_MSG Commit Message: PS8: Is there any verifiable constraints once a trace is added into the coordinator? If so, does it make sense to include a minimum smoke test scenarios for that? http://gerrit.cloudera.org:8080/#/c/22924/8//COMMIT_MSG@10 PS8, Line 10: Add coordinator trace Probably, that's done in some other patch, but since I don't see it in the gerrit patch, I'd rather ask right here :) Is it expected to be a notion of context at all as a higher level umbrella for the added coordinator's trace? I'm curious what the OTEL's Context entity is going to be mapped into in Impala. http://gerrit.cloudera.org:8080/#/c/22924/8/be/src/observe/otel-flags.cc File be/src/observe/otel-flags.cc: http://gerrit.cloudera.org:8080/#/c/22924/8/be/src/observe/otel-flags.cc@37 PS8, Line 37: rfind nit: it's a bit strange to see rfind() but not find() if trying to make sure the string starts with the specified suffix. Is this an attempt to do some extra validation on the string? http://gerrit.cloudera.org:8080/#/c/22924/8/be/src/observe/otel-flags.cc@43 PS8, Line 43: R"(^(https?://)" // http(s):// Why to have this if the string was already validated for the presence of such suffixes above? Maybe, it make sense to drop the duplicate validation at lines 37-40 if this regex validator is present? http://gerrit.cloudera.org:8080/#/c/22924/8/be/src/observe/otel-flags.cc@163 PS8, Line 163: DEFINE_validator(otel_retry_policy_max_attempts, [](const char* flagname, int32_t value) { : if (value < 0) { : LOG(ERROR) << "Flag '" << flagname << "' must be greater than or equal to 0."; : return false; : } : return true; : }); Is it possible to use uint32 flag for otel_retry_policy_max_attempts instead of adding this validator? http://gerrit.cloudera.org:8080/#/c/22924/8/be/src/observe/otel.cc File be/src/observe/otel.cc: http://gerrit.cloudera.org:8080/#/c/22924/8/be/src/observe/otel.cc@67 PS8, Line 67: opts.ssl_min_tls = FLAGS_otel_tls_minimum_version.empty() ? : "1.2" : FLAGS_otel_tls_minimum_version; Does this make sense to to set opts.ssl_min_tls at all if --otel_tls_minimum_version is empty or we want to enforce having 1.2 set as minimum all the time? http://gerrit.cloudera.org:8080/#/c/22924/8/be/src/observe/otel.cc@70 PS8, Line 70: opts.ssl_cipher = FLAGS_otel_ssl_ciphers.empty() ? "" : FLAGS_otel_ssl_ciphers; Could this be reduced to opts.ssl_cipher = FLAGS_otel_ssl_ciphers; ? http://gerrit.cloudera.org:8080/#/c/22924/8/be/src/observe/otel.cc@95 PS8, Line 95: provider nit: does it make sense to use std::move here as well? http://gerrit.cloudera.org:8080/#/c/22924/8/bin/impala-config.sh File bin/impala-config.sh: http://gerrit.cloudera.org:8080/#/c/22924/8/bin/impala-config.sh@210 PS8, Line 210: 1.20.0 They released 1.21.0 just end of May 2025: picking https://github.com/open-telemetry/opentelemetry-cpp/releases/tag/v1.21.0 If it makes sense, consider pickup up the most recent version unless there is some specific reason of not doing so. -- To view, visit http://gerrit.cloudera.org:8080/22924 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: Impala-ASF Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: Ie40b5cd33274df13f3005bf7a704299ebfff8a5b Gerrit-Change-Number: 22924 Gerrit-PatchSet: 8 Gerrit-Owner: gaurav singh <[email protected]> Gerrit-Reviewer: Alexey Serbin <[email protected]> Gerrit-Reviewer: Impala Public Jenkins <[email protected]> Gerrit-Reviewer: Jason Fehr <[email protected]> Gerrit-Reviewer: Michael Smith <[email protected]> Gerrit-Reviewer: gaurav singh <[email protected]> Gerrit-Comment-Date: Mon, 09 Jun 2025 22:56:07 +0000 Gerrit-HasComments: Yes
