mlevkov commented on PR #2925: URL: https://github.com/apache/iggy/pull/2925#issuecomment-4128950191
Thank you @spetz — excellent feedback across the board. Here's how I've grouped the 12 comments for implementation: ### Grouped by theme **A. Constants & naming** (straightforward) | # | Line | Change | |---|------|--------| | 1 | 77 | `Ndjson` → `NdJson` (`nd_json` in snake_case) | | 3 | 318 | `pub const` for `"data"`, `"iggy_payload_encoding"` field names | | 5 | 349 | Constants for all metadata field names (`iggy_checksum`, `iggy_origin_timestamp`, etc.) | | 11 | 1184 | Remove `// ── ... ──` comment sections; reorder helpers below callers | | 12 | 1669 | Use constants for field names in test assertions | **B. Strongly typed structs + enum methods** | # | Line | Change | |---|------|--------| | 2 | 295 | Move `content_type()` to `BatchMode` impl | | 4/6 | 340/378 | Replace `serde_json::json!` with serializable `MetadataEnvelope` struct | | 8 | 584 | `strum` Display on `BatchMode` for `mode_name` instead of string parameter | **C. Method signature cleanup** | # | Line | Change | |---|------|--------| | 9 | 444 | Use `self.client` directly instead of passing `client` parameter | **D. Design decisions (want to discuss)** | # | Line | Question | |---|------|----------| | 7 | 443 | `reqwest-middleware` — would replace hand-rolled retry with the workspace's middleware + retry policy. Leaning toward yes — aligns with the rest of the connectors runtime and gets retry improvements for free on workspace upgrades. | | 10 | 942 | UUID format — currently v8 (per hubcio's earlier request for proper RFC 4122 bits). Options: (a) v4 random UUID (loses correlation to iggy message ID), (b) v8 custom (current — preserves most of the ID but overwrites 6 bits), (c) simple hex without dashes (lossless but not a valid UUID), (d) user-configurable. What's your preference? | Will work through A → B → C → D in order. Addressing #10 and #7 last since they need alignment. -- 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]
