spetz commented on code in PR #4150:
URL: https://github.com/apache/iggy/pull/4150#discussion_r3997332029
##########
core/metadata/src/stm/stream.rs:
##########
@@ -2244,10 +2244,8 @@ impl StateHandler for UpdateTopicRequest {
stream.topic_index.remove(&topic.name);
topic.name = new_name_arc.clone();
- // Settings arrive only through the options block now, so the typed
- // fields are a projection of it and cannot drift. Absent means absent:
- // a client that sends just a rename leaves every setting alone, and
one
- // built before a key existed cannot erase it.
+ // Parsing treats default sentinels as absent for these typed fields,
Review Comment:
Same defect, other end. The replaced comment documents the divergence
("Parsing treats default sentinels as absent for these typed fields, while the
stored option map below retains their raw values") rather than reconciling the
two writes three lines apart.
##########
core/cli/src/args/topic.rs:
##########
@@ -166,18 +166,18 @@ pub(crate) struct TopicUpdateArgs {
pub(crate) topic_id: Identifier,
/// New name for the topic
pub(crate) name: String,
- /// Compression algorithm for the topic, set to "none" for no compression
+ /// Compression metadata (none or gzip). Payload compression is not
implemented
#[arg(value_parser = clap::value_parser!(CompressionAlgorithm),
verbatim_doc_comment)]
pub(crate) compression_algorithm: CompressionAlgorithm,
/// New max topic size in human-readable format like "unlimited" or "15GB"
///
- /// "server_default" or skipping parameter makes CLI to use server default
(from current server config) max topic size
- /// Can't be lower than segment size in the config.
+ /// Skipping this parameter or using "server_default" sends the legacy
zero sentinel. Pass an explicit value to avoid inconsistent option reporting.
Review Comment:
CLI help now tells the operator to work around a bug instead of fixing it:
"Skipping this parameter or using server_default sends the legacy zero
sentinel. Pass an explicit value to avoid inconsistent option reporting." Same
text at line 180 for `message_expiry`. The divergence is real.
`TopicCreateOptions::parse_committed` maps the sentinel to `None` at
`core/common/src/types/options/mod.rs:825`, while `resource_options_from_wire`
keeps the raw value and `topic.options.extend(updated_options)` stores it.
After `iggy topic update` without `-m`, the typed `max_topic_size` keeps its
old value while `GetTopic`'s option map reports `server_defaul`
##########
core/server/src/http/forward.rs:
##########
@@ -135,8 +136,13 @@ const RESPONSE_CAPACITY_HINT: usize = 64 * 1024;
/// applied op, not this follower's (the response layer only fills either when
/// absent); the applied op is also what this node records as the caller's
/// read-your-writes floor, so dropping it here would reopen the stale read.
-const RELAYED_RESPONSE_HEADERS: [HeaderName; 4] =
- [CONTENT_TYPE, RETRY_AFTER, VIEW_HEADER, APPLIED_OP_HEADER];
+const RELAYED_RESPONSE_HEADERS: [HeaderName; 5] = [
Review Comment:
`RELAYED_RESPONSE_HEADERS` grows to five entries. The doc block above it
explains why each of `content-type`, `retry-after`, `iggy-view` and
`iggy-applied-op` is relayed, and was not extended for `iggy-durability`.
##########
core/bench/src/utils/mod.rs:
##########
@@ -187,7 +190,11 @@ fn add_environment_variables(parts: &mut Vec<String>,
server_address: &str) {
if is_localhost {
let iggy_vars: Vec<_> = std::env::vars()
- .filter(|(k, _)| k.starts_with("IGGY_"))
+ .filter(|(name, _)| {
+ RUNTIME_ENV_VARS.contains(&name.as_str())
Review Comment:
The new allowlist keeps `ServerConfig` env mappings plus two hand-copied
shard variables, so `IGGY_CONFIG_PATH` and `IGGY_ENV_PATH` no longer reach the
report's reproduction command. Both change what server the run measured.
`SERVER_PROCESS_ENV_VARS` at `core/configs/src/server_config/server.rs:52`
already enumerates the non-`ServerConfig` set including those two.
##########
core/integration/src/bench_utils.rs:
##########
@@ -180,3 +180,92 @@ pub fn get_random_path() -> String {
.display()
.to_string()
}
+
+#[cfg(test)]
+mod tests {
Review Comment:
Two new tests that spawn a real server and run a full benchmark sit in the
library's `#[cfg(test)]` module, so they land in the `--lib` binary rather than
the `tests/mod.rs` binary. They get none of the `#[ctor]` harness from
`core/integration/tests/mod.rs:169`: no tracing subscriber, no per-test file
under `test_logs/`, no failure log dump on panic. A CI failure in either
produces no diagnostic output.
##########
README.md:
##########
@@ -89,10 +89,10 @@ The name is an abbreviation for the Italian Greyhound -
small yet extremely fast
are reserved for future disk/network compression support; use message headers
for manual compression today (see
`examples/rust/src/message-headers/message-compression`).
- Optional **data backups and archiving** to disk or **S3** compatible cloud
storage (e.g. AWS S3)
-- Support for **OpenTelemetry** logs & traces + Prometheus metrics
+- Prometheus metrics and **OpenTelemetry** logs & traces in the connectors
runtime. Server OTLP export is unavailable pending runtime integration.
Review Comment:
Claims Prometheus metrics and OpenTelemetry exist only in the connectors
runtime and that server OTLP export is unavailable. The server calls
`logging.late_init(..., &TelemetrySettings::from(&config.telemetry))` at
`core/server/src/boot/mod.rs:176` with working OTLP gRPC and HTTP exporters,
and `core/server/src/http/metrics.rs` serves the Prometheus registry. The PR's
own new test at `core/integration/tests/server/telemetry.rs:24` boots the
server with `IGGY_TELEMETRY_LOGS_TRANSPORT=http` against a mock collector, so
the PR contradicts itself.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
