You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@iggy.apache.org by "hubcio (via GitHub)" <gi...@apache.org> on 2026/09/12 11:05:46 UTC
[PR] fix(server): preserve write responses and validate CLI input [iggy]
hubcio opened a new pull request, #4150:
URL: https://github.com/apache/iggy/pull/4150
Forwarding can lose durability headers, accepted CLI input can panic
or parse incorrectly, and benchmarks can expose secrets or join the
wrong consumer group.
Preserve response headers, validate CLI input, redact benchmark
metadata and select the matching group. Avoid the telemetry reload
deadlock and align protocol, configuration and command guidance with
the implemented behavior.
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(server): preserve write responses and validate CLI input [iggy]
Posted by "spetz (via GitHub)" <gi...@apache.org>.
spetz merged PR #4150:
URL: https://github.com/apache/iggy/pull/4150
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(server): preserve write responses and validate CLI input [iggy]
Posted by "numinnex (via GitHub)" <gi...@apache.org>.
numinnex commented on PR #4150:
URL: https://github.com/apache/iggy/pull/4150#issuecomment-5676811759
/dst
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(server): preserve write responses and validate CLI input [iggy]
Posted by "codecov[bot] (via GitHub)" <gi...@apache.org>.
codecov[bot] commented on PR #4150:
URL: https://github.com/apache/iggy/pull/4150#issuecomment-5645506799
## [Codecov](https://app.codecov.io/gh/apache/iggy/pull/4150?dropdown=coverage&src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) Report
:white_check_mark: All modified and coverable lines are covered by tests.
:white_check_mark: Project coverage is 86.00%. Comparing base ([`dc2b382`](https://app.codecov.io/gh/apache/iggy/commit/dc2b382097ed7fb6cfeb8a31fed0fbc74af4433e?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)) to head ([`fd13148`](https://app.codecov.io/gh/apache/iggy/commit/fd13148027b7c5f3a2efe35bb5f59b6b2d6da0e1?dropdown=coverage&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)).
:warning: Report is 1 commits behind head on master.
<details><summary>Additional details and impacted files</summary>
```diff
@@ Coverage Diff @@
## master #4150 +/- ##
============================================
- Coverage 86.38% 86.00% -0.39%
Complexity 1455 1455
============================================
Files 1259 1257 -2
Lines 205992 205724 -268
Branches 171193 171130 -63
============================================
- Hits 177955 176940 -1015
- Misses 23597 24339 +742
- Partials 4440 4445 +5
```
| [Components](https://app.codecov.io/gh/apache/iggy/pull/4150/components?src=pr&el=components&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [Rust Core](https://app.codecov.io/gh/apache/iggy/pull/4150/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `87.33% <ø> (+<0.01%)` | :arrow_up: |
| [Java SDK](https://app.codecov.io/gh/apache/iggy/pull/4150/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `67.58% <ø> (ø)` | |
| [C# SDK](https://app.codecov.io/gh/apache/iggy/pull/4150/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `76.97% <ø> (ø)` | |
| [Python SDK](https://app.codecov.io/gh/apache/iggy/pull/4150/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `91.34% <ø> (ø)` | |
| [PHP SDK](https://app.codecov.io/gh/apache/iggy/pull/4150/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `85.65% <ø> (ø)` | |
| [Node SDK](https://app.codecov.io/gh/apache/iggy/pull/4150/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `89.95% <ø> (-6.23%)` | :arrow_down: |
| [Go SDK](https://app.codecov.io/gh/apache/iggy/pull/4150/components?src=pr&el=component&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | `69.40% <ø> (ø)` | |
| [Files with missing lines](https://app.codecov.io/gh/apache/iggy/pull/4150?dropdown=coverage&src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | Coverage Δ | |
|---|---|---|
| [core/binary\_protocol/src/consensus/header.rs](https://app.codecov.io/gh/apache/iggy/pull/4150?src=pr&el=tree&filepath=core%2Fbinary_protocol%2Fsrc%2Fconsensus%2Fheader.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29yZS9iaW5hcnlfcHJvdG9jb2wvc3JjL2NvbnNlbnN1cy9oZWFkZXIucnM=) | `81.87% <ø> (ø)` | |
| [core/binary\_protocol/src/consensus/operation.rs](https://app.codecov.io/gh/apache/iggy/pull/4150?src=pr&el=tree&filepath=core%2Fbinary_protocol%2Fsrc%2Fconsensus%2Foperation.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29yZS9iaW5hcnlfcHJvdG9jb2wvc3JjL2NvbnNlbnN1cy9vcGVyYXRpb24ucnM=) | `97.10% <ø> (ø)` | |
| [...inary\_protocol/src/requests/topics/update\_topic.rs](https://app.codecov.io/gh/apache/iggy/pull/4150?src=pr&el=tree&filepath=core%2Fbinary_protocol%2Fsrc%2Frequests%2Ftopics%2Fupdate_topic.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29yZS9iaW5hcnlfcHJvdG9jb2wvc3JjL3JlcXVlc3RzL3RvcGljcy91cGRhdGVfdG9waWMucnM=) | `100.00% <ø> (ø)` | |
| [...y\_protocol/src/responses/messages/send\_messages.rs](https://app.codecov.io/gh/apache/iggy/pull/4150?src=pr&el=tree&filepath=core%2Fbinary_protocol%2Fsrc%2Fresponses%2Fmessages%2Fsend_messages.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29yZS9iaW5hcnlfcHJvdG9jb2wvc3JjL3Jlc3BvbnNlcy9tZXNzYWdlcy9zZW5kX21lc3NhZ2VzLnJz) | `97.85% <ø> (ø)` | |
| [core/binary\_protocol/src/version.rs](https://app.codecov.io/gh/apache/iggy/pull/4150?src=pr&el=tree&filepath=core%2Fbinary_protocol%2Fsrc%2Fversion.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29yZS9iaW5hcnlfcHJvdG9jb2wvc3JjL3ZlcnNpb24ucnM=) | `99.22% <ø> (ø)` | |
| [core/cli/src/args/message.rs](https://app.codecov.io/gh/apache/iggy/pull/4150?src=pr&el=tree&filepath=core%2Fcli%2Fsrc%2Fargs%2Fmessage.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29yZS9jbGkvc3JjL2FyZ3MvbWVzc2FnZS5ycw==) | `66.12% <ø> (ø)` | |
| [core/cli/src/args/mod.rs](https://app.codecov.io/gh/apache/iggy/pull/4150?src=pr&el=tree&filepath=core%2Fcli%2Fsrc%2Fargs%2Fmod.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29yZS9jbGkvc3JjL2FyZ3MvbW9kLnJz) | `78.57% <ø> (ø)` | |
| [core/cli/src/args/topic.rs](https://app.codecov.io/gh/apache/iggy/pull/4150?src=pr&el=tree&filepath=core%2Fcli%2Fsrc%2Fargs%2Ftopic.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29yZS9jbGkvc3JjL2FyZ3MvdG9waWMucnM=) | `100.00% <ø> (ø)` | |
| [...e/cli/src/commands/binary\_message/send\_messages.rs](https://app.codecov.io/gh/apache/iggy/pull/4150?src=pr&el=tree&filepath=core%2Fcli%2Fsrc%2Fcommands%2Fbinary_message%2Fsend_messages.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29yZS9jbGkvc3JjL2NvbW1hbmRzL2JpbmFyeV9tZXNzYWdlL3NlbmRfbWVzc2FnZXMucnM=) | `85.71% <ø> (+3.25%)` | :arrow_up: |
| [core/common/src/traits/binary\_impls/messages.rs](https://app.codecov.io/gh/apache/iggy/pull/4150?src=pr&el=tree&filepath=core%2Fcommon%2Fsrc%2Ftraits%2Fbinary_impls%2Fmessages.rs&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache#diff-Y29yZS9jb21tb24vc3JjL3RyYWl0cy9iaW5hcnlfaW1wbHMvbWVzc2FnZXMucnM=) | `83.04% <ø> (ø)` | |
| ... and [18 more](https://app.codecov.io/gh/apache/iggy/pull/4150?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache) | |
... and [54 files with indirect coverage changes](https://app.codecov.io/gh/apache/iggy/pull/4150/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=apache)
</details>
<details><summary> :rocket: New features to boost your workflow: </summary>
- :snowflake: [Test Analytics](https://docs.codecov.com/docs/test-analytics): Detect flaky tests, report on failures, and find test suite problems.
- :package: [JS Bundle Analysis](https://docs.codecov.com/docs/javascript-bundle-analysis): Save yourself from yourself by tracking and limiting bundle sizes in JS merges.
</details>
--
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] fix(server): preserve write responses and validate CLI input [iggy]
Posted by "spetz (via GitHub)" <gi...@apache.org>.
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: commits-unsubscribe@iggy.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org