Repository navigation
Conversation
✅ Deploy Preview for elastic-elion-a958b6 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
gangtao
left a comment
There was a problem hiding this comment.
Review of the 3.3.4 release notes, checked against git log v3.3.3..v3.3.4 in proton-enterprise and the published artifacts. Inline comments cover the lines in this diff; the items below are in files outside the diff that this PR leaves inconsistent with the new changelog.
docs/kafka-schema-registry.md:56 and docs/shared/kafka-external-stream.md:243 — both reference pages still state consume_schema_strategy='single' is the default. In v3.3.4 the default is empty and resolved from subject_name_strategy (all for unset/TopicNameStrategy, single otherwise; proton-enterprise #12502, commit 36dbbe4c94, getEffectiveConsumeSchemaStrategy()). The changelog added here and the reference pages now disagree.
schema_message_name — announced in this changelog (#12479) but has no entry in any reference page (accepted forms: top-level message name or fully qualified name; defaults to the schema's first message). The single strategy's sibling-message filter (#12491) is likewise undocumented.
docs/enterprise-v3.4-whats-new.md:79 — states ttl_move_grace_seconds defaults to 300. This PR correctly says 1 hour for 3.3.4 (#12493; MergeTreeSettings.h at tag v3.3.4 is 3600), but neither page version-qualifies the value and there is no reference entry, so the two release pages contradict each other.
Spellcheck note: could not run yarn spellcheck locally (no node_modules); the only out-of-dictionary words in the new text (Raft, sn, epoch) already appear in the 3.3.3 section.
…ject_name_strategy (#12502) + upgrade notes
…ystem log retention + rename note, ttl_move_grace_seconds 3.4.1 default
|
Status of the review items that are not inline edits (updated in place, latest state): Done in this PR
Handled in #701 (merge together with this PR)
Follow-up PR
|
…ds entry (review)
gangtao
left a comment
There was a problem hiding this comment.
Re-review of the updated branch. The earlier blocking items (consume_schema_strategy derivation, #12491, chart/tarball availability) are addressed. Remaining points inline. The two reference-doc gaps are fixed by #701, so please merge #699 and #701 together rather than #699 alone.
Nit: trailing periods are inconsistent across the new Features bullets (#12479 and #12367 end without one, the rest end with one); the 3.3.3 section uses no trailing periods.
| This release consolidates timeplusd changes introduced since 3.3.3. For changes already published in 3.3.3, see the [3.3.3 changelog](#changelog_3_3_3) below. | ||
|
|
||
| **Features and Enhancements** | ||
| * Kafka external streams with schema registry: support Protobuf schemas that import other schemas (references), and add two new settings — `schema_message_name` to pick which top-level message the stream's columns are inferred from, and `consume_schema_strategy` (`single` / `all` / `raw`) to control how messages with different schema IDs are handled (#12479) |
There was a problem hiding this comment.
schema_message_name is introduced here but has no entry in the Kafka external stream settings reference on main. #701 adds the #### schema_message_name section; same note as above, merge together.
There was a problem hiding this comment.
Handled in #701: schema_message_name now has a #### schema_message_name section in docs/shared/kafka-external-stream.md (and in the syntax blocks), including the single sibling-message filtering (#12491). To be merged together with this PR.
|
|
||
| #### Changelog {#changelog_3_3_4} | ||
|
|
||
| This release consolidates timeplusd changes introduced since 3.3.3. For changes already published in 3.3.3, see the [3.3.3 changelog](#changelog_3_3_3) below. |
There was a problem hiding this comment.
This changelog lists many timeplusd fixes (#12429, #12431, #12432, #12439, #12456, #12465, #12473, #12481, #12491, #12502, #12441) that are absent from the 3.4.1 changelog, and neither page says whether 3.4.1 contains them. An operator on 3.3.4 who upgrades to 3.4.1 because it is the newer line could regress on the Raft snapshot-epoch fix or the Protobuf schema-reference support. One sentence here noting that 3.4.1 predates these changes (and which 3.4.x will carry them) would remove the ambiguity.
There was a problem hiding this comment.
Added in fd3b3f3: a paragraph under the changelog intro says 3.4.1 predates several of these changes (e.g. #12432 Raft snapshot epoch, #12479 Protobuf schema references) and that the changelog of the 3.4.x release should be checked before moving from 3.3.4. I checked with git compare v3.3.3...v3.4.1: none of the PRs you listed is in 3.4.1; they first appear in v3.4.2-rc.2. I did not name the 3.4.x release, because 3.4.2 is still a release candidate.
| * Downgrade: streams that set `consume_schema_strategy` explicitly cannot be loaded by 3.3.3 (unknown setting). Streams that rely on the default can. | ||
| * Merge TTL-expired parts before moving them to a no-merge volume (e.g. S3 cold tier), so they land as fewer, larger parts (#12269). A new stream setting `ttl_move_grace_seconds` additionally holds freshly written lone parts on the local volume so late batches can still merge before the move (#12360); its default is 1 hour (#12493). | ||
| * Apply merge selector limits (`max_bytes_to_merge_at_max_space_in_pool`, `max_parts_to_merge_at_once`) to `OPTIMIZE STREAM ... PARTITION p` without `FINAL`; it now triggers one bounded merge per call instead of merging the whole partition, and compacts `prefer_not_to_merge` volumes (#12367) | ||
| * Reduce default system log retention and disable `log_query_threads` by default (#12441). The new retention is 3 days for `trace_log`, 7 days for `query_thread_log`, 14 days for `part_log` and 7 days for `stream_metric_log`, and `log_query_threads` now defaults to `false`. On the first start after the upgrade, the existing `trace_log`, `query_thread_log` and `part_log` tables (whose definition changed) are renamed to `<name>_N` and kept rather than dropped, so their disk space is not reclaimed until you drop the renamed tables; `stream_metric_log` is dropped and re-created with the new TTL. Impact: with the old defaults, system log tables could consume most of a node's local disk. |
There was a problem hiding this comment.
This wording matches the code: SystemLog::prepareTable() renames MergeTree-backed logs to <name>_N, and drops/re-creates the Stream-backed stream_metric_log because rename is not implemented for streams. Note that the 2.8 note for the same change (docs/enterprise-v2.8.md:56, #12440) says all existing log tables are renamed and kept, which is imprecise for stream_metric_log. Not a blocker for this PR; worth a follow-up fix on the 2.8 page and a cross-reference between #12441 and #12440.
There was a problem hiding this comment.
Thanks for confirming the code behavior. Added the cross-reference in 2c09202: the #12441 entry now ends with "The same change shipped in 2.8.49 as #12440" and links to the 2.8.21 release notes. The 2.8 side is fixed in #703 (stream_metric_log is dropped and re-created, the other three log tables are renamed), and it points back to #12441.
| * When `consume_schema_strategy` is not set, its default now follows `subject_name_strategy`: `'all'` for `TopicNameStrategy` (or when `subject_name_strategy` is not set), and `'single'` for `RecordNameStrategy`, `TopicRecordNameStrategy` and custom strategies (#12502, #12500). An explicit value always wins, and the resolved default is not stored in the stream DDL. Upgrade notes from 3.3.3: | ||
| * Avro streams: no change. Streams using `RecordNameStrategy` / `TopicRecordNameStrategy` still consume only the record type of `schema_subject_name`, and streams using `TopicNameStrategy` still consume every message. | ||
| * Protobuf streams using `RecordNameStrategy` / `TopicRecordNameStrategy`: 3.3.3 consumed every message of the topic regardless of its schema; 3.3.4 consumes only the messages of the selected subject (and of the message chosen by `schema_message_name`) and silently skips the rest. To keep the 3.3.3 behavior, set `consume_schema_strategy = 'all'`. | ||
| * `SELECT count()` on a schema-registry stream that resolves to `'single'` now decodes and filters, so it returns the number of rows the stream yields instead of the raw number of Kafka messages. |
There was a problem hiding this comment.
The SELECT count() filtering behavior is already stated in the #12487/#12488 bullet above (line 47) with slightly different wording. Suggest keeping one mention so the two copies do not drift.
There was a problem hiding this comment.
Fixed in fd3b3f3: removed the SELECT count() sub-bullet from the upgrade notes. The behavior is now stated once, in the #12487/#12488 bullet.
…s, dedupe count() note, trailing periods
…etention change (#12440)
No description provided.