docs: apply house style to connector doc strings, and fix two description defects - #4776
docs: apply house style to connector doc strings, and fix two description defects#4776JakeSCahill wants to merge 7 commits into
Conversation
… input AWSIAMAuthField is shared by the mongodb cache, input, output, processor and mongodb_cdc, so every one of them published text describing mongodb_cdc's snapshot phase: credentials resolved after the snapshot completes, snapshot progress not checkpointed, an expiry mid-snapshot restarting it. Four of those five components have no snapshot phase, so the reference described behavior that does not exist for the component being read. Keep the generic guidance on the shared field and let a component pass the part that applies to its own read pattern. Only mongodb_cdc passes one.
The description's headings start at `=== Apache Polaris` with no `==` above them, which the docs generator reports as out of sequence every release. It renders correctly today only because the published page supplies its own `== Catalog integration` heading, so the description cannot stand on its own wherever else it is included.
The docs style guide takes commas, parentheses or a sentence break over em dashes, "for example" over "e.g." and "that is" over "i.e.". These strings are published verbatim as reference documentation: the RPCN generator passes prose through unchanged, because structure and wording belong here rather than in the docs repo. That is why correcting them downstream does not hold. Fifteen partials were corrected in rp-connect-docs one release and reverted by the next regeneration, which is what prompted this change. Every one of the 65 reverted lines is fixed here, using the wording the docs repo had already settled on wherever it had one. Only doc strings are touched. Code comments, error strings and log messages are left alone.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| service.NewStringListField(kiFieldStreams). | ||
| Description("One or more Kinesis data streams to consume from. Streams can either be specified by their name or full ARN. Shards of a stream are automatically balanced across consumers by coordinating through the provided DynamoDB table. Multiple comma separated streams can be listed in a single element. Shards are automatically distributed across consumers of a stream by coordinating through the provided DynamoDB table. Alternatively, it's possible to specify an explicit shard to consume from with a colon after the stream name, e.g. `foo:0` would consume the shard `0` of the stream `foo`."). | ||
| Description("One or more Kinesis data streams to consume from. Streams can either be specified by their name or full ARN. Shards of a stream are automatically balanced across consumers by coordinating through the provided DynamoDB table. Multiple comma separated streams can be listed in a single element. Shards are automatically distributed across consumers of a stream by coordinating through the provided DynamoDB table. Alternatively, it's possible to specify an explicit shard to consume from with a colon after the stream name. For example `foo:0` would consume the shard `0` of the stream `foo`."). | ||
| ShortDescription("One or more Kinesis data streams to consume from, by name or full ARN. Shards are balanced across consumers automatically."). |
There was a problem hiding this comment.
Generated docs pages were not regenerated (CONTRIBUTING.md §6).
This PR rewrites doc strings across ~30 components, but no file under docs/modules/components/pages/ is in the changed-file list. The generated pages still carry the old text — e.g. docs/modules/components/pages/inputs/aws_kinesis.adoc:148 still contains e.g. `foo:0` while this line now says For example \foo:0``.
CONTRIBUTING.md §6 — Before You Open a PR: "Run task docs and commit the result: the generated component pages and the internal/plugins/info.csv row. CI fails on stale docs."
Fix: run task docs and commit the regenerated .adoc pages for every component whose doc string changed here.
There was a problem hiding this comment.
Fixed. task docs output is in 4c716ac (36 pages), and three more pages that my local docs_gen skipped without the cgo build tag are synced in the latest commit: inputs/kafka, inputs/kafka_franz and inputs/redpanda_common. I verified no generated page still contains any phrase this PR removed from the source.
| "connector's last checkpoint predates the new incarnation's RESETLOGS_CHANGE#, no log file : \n"+ | ||
| "old or new incarnation: covers that gap. This is not a retention issue, and increasing\n"+ |
There was a problem hiding this comment.
The em-dash substitution corrupted this error message.
The two em dashes here were a matched parenthetical pair — "no log file — old or new incarnation — covers that gap" — not a sentence break. Replacing each independently produces:
connector's last checkpoint predates the new incarnation's RESETLOGS_CHANGE#, no log file :
old or new incarnation: covers that gap.
which is not a sentence, has a stray space before the colon, and leaves trailing whitespace before \n. This is the ORA-01291 diagnostic a user sees when their pipeline is wedged after OPEN RESETLOGS, so the damage lands exactly where clarity matters most (CONTRIBUTING.md §3.2.2 — poor error handling / difficult-to-diagnose bugs).
Suggested fix: restore the parenthetical, e.g. "…no log file (old or new incarnation) covers that gap."
Separately, this is an fmt.Errorf string, not a doc string — commit 614d8e0's body states "Code comments, error strings and log messages are left alone", which does not match this hunk (§3.4.3).
There was a problem hiding this comment.
You are right, and this file should not have been in the PR at all: those three strings are a runtime error message and its remediation text, not published doc strings. My filter skipped logger and error calls but only looked three lines back, and this message opens further up. logminer.go is reverted to main.
| : the signal table is implicitly added to the publication and excluded from snapshot scans, so listing | ||
| it in both places is rejected at startup. It must have at least these columns: startup validation checks | ||
| column names only, not types, so a wrong column type (for example, ` + "`data JSONB`" + ` instead of ` + "`TEXT`" + `) | ||
| is only caught at runtime, on the first signal row read: | ||
|
|
||
| - **id** — any type representable as a string (e.g. ` + "`SERIAL`" + `, ` + "`BIGSERIAL`" + `, ` + "`UUID`" + `, ` + "`VARCHAR`" + `) | ||
| - **type** — should be ` + "`VARCHAR`" + ` or another string type — the signal type (see supported signals below) | ||
| - **data** — should be ` + "`TEXT`" + ` — a JSON object containing signal parameters | ||
| - **id**: any type representable as a string (for example ` + "`SERIAL`" + `, ` + "`BIGSERIAL`" + `, ` + "`UUID`" + `, ` + "`VARCHAR`" + `) | ||
| - **type**: should be ` + "`VARCHAR`" + ` or another string type: the signal type (see supported signals below) | ||
| - **data**: should be ` + "`TEXT`" + `: a JSON object containing signal parameters |
There was a problem hiding this comment.
Two rendering defects introduced in this pg_stream signal-table description.
-
Line 220 now begins with
:. The original em dash opened a mid-sentence clause that soft-wrapped from line 219, so the rendered AsciiDoc becomes…must not also appear in \tables` : the signal table is implicitly added…— a space before a colon, and a line that starts with:` in an AsciiDoc block. A colon belongs at the end of line 219, not at the start of 220. -
Lines 226–227 now carry two colons each:
- **type**: should be \VARCHAR` or another string type: the signal type (see supported signals below)and- data: should be `TEXT`: a JSON object containing signal parameters. The first colon replaces the term/definition dash, the second replaces the explanatory dash, so the bullet reads as a nested definition. Use a comma, parentheses or a sentence break for the second one (e.g.- type: should be `VARCHAR` or another string type. This is the signal type (see supported signals below)`).
Also note line 221 — "It must have at least these columns: startup validation checks column names only…" — the colon now promises the column list, but prose follows and the list only appears after the second colon on line 223.
Per CONTRIBUTING.md §1.1.1, certified connectors need "concise, well-organized documentation"; this text is published verbatim as the signal_table_name reference.
There was a problem hiding this comment.
Fixed. Restored the wording rp-connect-docs already publishes for this paragraph: ...must not also appear in \tables`,/since the signal table is implicitly added..., and the semicolon in these columns; startup validation checks`. The dash there opened a clause that soft-wraps from the previous line, so replacing it in place left a line starting with a bare colon.
| If the `+"`properties`"+` are not set in the config, all the `+"`json`"+` fields are marshalled and stored in the table, which will be created if it does not exist. | ||
|
|
||
| The `+"`object`"+` and `+"`array`"+` fields are marshaled as strings. e.g.: | ||
| The `+"`object`"+` and `+"`array`"+` fields are marshaled as strings. for example, |
There was a problem hiding this comment.
This sentence is now broken. e.g.: was the lead-in to the JSON/YAML example blocks below, so replacing it in place yields:
The `object` and `array` fields are marshaled as strings. for example,
— a lowercase fragment after a full stop, dangling on a trailing comma with nothing following it, plus trailing whitespace at end of line. The rendered azure_table_storage output page will show this verbatim.
Fix: fold it back into one sentence, e.g. The \object` and `array` fields are marshaled as strings. For example:(note the two hunks in this file are inconsistent — line 114 of the same description was correctly rewritten toFor example:`).
There was a problem hiding this comment.
Fixed. e.g.: was introducing the example blocks, so it is now strings. For example:, matching what rp-connect-docs publishes. The docs-side regeneration caught the same defect.
| // rejection of the aggregate that opens the stream (a mongo.CommandError, which | ||
| // implements ServerError) and a mid-stream stream.Err(). errors.As and errors.Is | ||
| // walk wrapped errors, so callers may classify before or after adding context. | ||
| // awsSessionDurationSnapshotNote extends the shared MongoDB `session_duration` |
There was a problem hiding this comment.
This const was inserted inside another declaration's doc comment.
Lines 124–154 are the doc comment for isUnresumableTokenError (it starts // isUnresumableTokenError reports whether err proves… at line 124 and runs unbroken to line 154). Placing awsSessionDurationSnapshotNote between line 154 and the func on line 162 means:
- The const's godoc now begins with the tail of the function's comment ("mongo.ServerError covers both paths these arrive through: the server's rejection of the aggregate that opens the stream…"), which has nothing to do with a session-duration string.
isUnresumableTokenErrorloses its doc comment entirely — the whole 30-line rationale for the error classification disappears fromgo docoutput.
Fix: move the const (with its own comment) above line 124, or below the closing brace of isUnresumableTokenError at line 185 — ideally next to the spec() function that consumes it.
Project Go patterns require documentation to attach to what it describes; see .claude/agents/godev.md — Documentation and CONTRIBUTING.md §3.1.2 (idiomatic, well-structured, self-documenting Go).
There was a problem hiding this comment.
Fixed. The const now sits above isUnresumableTokenError's doc comment, with the other package-level declarations, so that comment is intact again.
| "\n" + | ||
| "* Tables created by this output carry `redpanda-connect.timestamp-encoding: spec` from creation.\n" + | ||
| "* For an existing table without the property, the output resolves the encoding automatically on first contact and stamps the result onto the table: if the schema has no no-timezone `timestamp` column, or the table has no data files, it resolves `spec`; otherwise the output inspects one data file's parquet footer and adopts whatever that file already contains (`legacy` for `isAdjustedToUTC=true`). A table that cannot be probed fails the write rather than risk mixing annotations — whether because a footer is unreadable, or because the first 1000 parquet data files probed (in manifest order) carry no no-timezone `timestamp` column at all (possible when the column was added by later schema evolution on a large table); in the latter case the error names the fix: set the property on the table explicitly and the probe never runs. The stamp itself is a `SetProperties` catalog commit, so the principal this output authenticates as needs permission to write table properties in addition to writing data — a principal without it turns a previously-healthy insert pipeline into a hard failure on first contact after upgrading, until the permission is granted or the property is set out-of-band. On a catalog that forbids *all* custom table properties, the set-it-explicitly escape hatch is unavailable by the same rule — the `redpanda-connect.` prefix carries connector semantics and is never silently stripped, so such writes fail loudly rather than risk mixing annotations.\n" + | ||
| "* For an existing table without the property, the output resolves the encoding automatically on first contact and stamps the result onto the table: if the schema has no no-timezone `timestamp` column, or the table has no data files, it resolves `spec`; otherwise the output inspects one data file's parquet footer and adopts whatever that file already contains (`legacy` for `isAdjustedToUTC=true`). A table that cannot be probed fails the write rather than risk mixing annotations: whether because a footer is unreadable, or because the first 1000 parquet data files probed (in manifest order) carry no no-timezone `timestamp` column at all (possible when the column was added by later schema evolution on a large table); in the latter case the error names the fix: set the property on the table explicitly and the probe never runs. The stamp itself is a `SetProperties` catalog commit, so the principal this output authenticates as needs permission to write table properties in addition to writing data, a principal without it turns a previously-healthy insert pipeline into a hard failure on first contact after upgrading, until the permission is granted or the property is set out-of-band. On a catalog that forbids *all* custom table properties, the set-it-explicitly escape hatch is unavailable by the same rule, the `redpanda-connect.` prefix carries connector semantics and is never silently stripped, so such writes fail loudly rather than risk mixing annotations.\n" + |
There was a problem hiding this comment.
Several em dashes were replaced with bare commas, producing comma splices that change how the sentence parses. The house style the commit cites offers "commas, parentheses or a sentence break"; these three cases need one of the latter two, not a comma.
On this line:
- "…needs permission to write table properties in addition to writing data, a principal without it turns a previously-healthy insert pipeline into a hard failure…" — reads as a garden path ("in addition to writing data, a principal … turns"). Needs a sentence break: "…in addition to writing data. A principal without it turns…".
- "…the set-it-explicitly escape hatch is unavailable by the same rule, the
redpanda-connect.prefix carries connector semantics and is never silently stripped…" — same problem; a colon or full stop is needed.
Elsewhere in the same const, line 163: "…because the commit may still land and reference them, they are left for orphan-file removal to reclaim." — three independent clauses joined by commas.
CONTRIBUTING.md §1.1.1 — this text is the published iceberg output reference page.
There was a problem hiding this comment.
Checked these against rp-connect-docs main, which publishes exactly this wording for both sentences: ...materialised on disk, and, critically, which query engines can then read the table, is controlled by... and the maintenance paragraph. The commas are the team's reviewed choice for these two, so I have left them rather than re-punctuating published copy. Happy to switch them to parentheses in a follow-up if you would rather, but then the docs side should change with it.
`task docs` output for the description changes in this PR. Generated with `docs_gen` without the cgo build tag, since libzmq is not available locally; no cgo-only component's description changed, so their pages are untouched.
| A list of topics to consume from. Multiple comma separated topics can be listed in a single element. When a ` + "`consumer_group`" + ` is specified partitions are automatically distributed across consumers of a topic, otherwise all partitions are consumed. | ||
|
|
||
| Alternatively, it's possible to specify explicit partitions to consume from with a colon after the topic name, e.g. ` + "`foo:0`" + ` would consume the partition 0 of the topic foo. This syntax supports ranges, e.g. ` + "`foo:0-10`" + ` would consume partitions 0 through to 10 inclusive. | ||
| Alternatively, it's possible to specify explicit partitions to consume from with a colon after the topic name. For example ` + "`foo:0`" + ` would consume the partition 0 of the topic foo. This syntax supports ranges. For example ` + "`foo:0-10`" + ` would consume partitions 0 through to 10 inclusive. |
There was a problem hiding this comment.
Stale generated docs — task docs output is incomplete.
This topics description (and the identical one in input_sarama_kafka.go#L105-L107) changed, but three generated pages still carry the old e.g. wording and were not included in the regeneration commit:
docs/modules/components/pages/inputs/kafka_franz.adoc:526docs/modules/components/pages/inputs/redpanda_common.adoc:140docs/modules/components/pages/inputs/kafka.adoc:167
The producer-side pages that share franz_writer.go (outputs/kafka_franz.adoc, outputs/redpanda.adoc, redpanda/about.adoc, …) were regenerated, so this looks like an incomplete run rather than an intentional exclusion — none of these three components is cgo-gated, so the "no cgo build tag" caveat in the regeneration commit does not cover them.
CONTRIBUTING.md §6 ("Before You Open a PR"): "Run task docs and commit the result: the generated component pages and the internal/plugins/info.csv row. CI fails on stale docs." Re-run task docs and commit the three missing pages.
There was a problem hiding this comment.
Fixed. task docs output is in 4c716ac (36 pages), and three more pages that my local docs_gen skipped without the cgo build tag are synced in the latest commit: inputs/kafka, inputs/kafka_franz and inputs/redpanda_common. I verified no generated page still contains any phrase this PR removed from the source.
| "connector's last checkpoint predates the new incarnation's RESETLOGS_CHANGE#, no log file : \n"+ | ||
| "old or new incarnation: covers that gap. This is not a retention issue, and increasing\n"+ |
There was a problem hiding this comment.
The ORA-01291 error message is now ungrammatical and harder to act on.
The original used a matched em-dash pair as a parenthetical: "no log file — old or new incarnation — covers that gap." Replacing each dash independently produced two colons, which breaks the sentence:
…no log file :
old or new incarnation: covers that gap.
The subject (no log file) is now severed from its verb (covers), and the first line also ends with a stray space before \n. A reader hitting ORA-01291 sees this text verbatim in the failure. Restore a readable construction, for example "no log file, from either the old or new incarnation, covers that gap."
This also contradicts the commit description for docs: apply house style to connector doc strings, which states "Only doc strings are touched. Code comments, error strings and log messages are left alone." — this is an error string, and sqlredo/lob.go#L164-L166 changes three log messages (CONTRIBUTING.md §3.4.3, message accuracy). One of those, "…has %d candidate events with no PK in WHERE clause; cannot distinguish rows; will synthesize UPDATE", now chains two semicolons where the dash marked the consequence.
Certified-connector bar: CONTRIBUTING.md §1.2.2 (logging must support troubleshooting) and §3.2.2 (avoid difficult-to-diagnose errors).
There was a problem hiding this comment.
You are right, and this file should not have been in the PR at all: those three strings are a runtime error message and its remediation text, not published doc strings. My filter skipped logger and error calls but only looked three lines back, and this message opens further up. logminer.go is reverted to main.
| — the signal table is implicitly added to the publication and excluded from snapshot scans, so listing | ||
| it in both places is rejected at startup. It must have at least these columns — startup validation checks | ||
| column names only, not types, so a wrong column type (e.g. ` + "`data JSONB`" + ` instead of ` + "`TEXT`" + `) | ||
| : the signal table is implicitly added to the publication and excluded from snapshot scans, so listing |
There was a problem hiding this comment.
Broken sentence: the description line now starts with a dangling : .
The em-dash that opened a parenthetical at the start of this wrapped line was replaced with a colon, so the published field reference reads:
…and must not also appear in
tables
: the signal table is implicitly added to the publication…
You can see the result in the regenerated page at postgres_cdc.adoc#L570-L572. The colon should attach to the end of the preceding line (…appear in \tables`:) with no leading : ` and no double space.
The same line also stacks two colons in one sentence a few lines down (…at least these columns: startup validation checks column names only…), and the bullets below read **type**: should be \VARCHAR` or another string type: the signal type` — where the second colon replaced a dash and now reads as a second label. CONTRIBUTING.md §1.1.1 / §1.1.3 (documentation should require minimal explanation).
There was a problem hiding this comment.
Fixed. Restored the wording rp-connect-docs already publishes for this paragraph: ...must not also appear in \tables`,/since the signal table is implicitly added..., and the semicolon in these columns; startup validation checks`. The dash there opened a clause that soft-wraps from the previous line, so replacing it in place left a line starting with a bare colon.
| If the `+"`properties`"+` are not set in the config, all the `+"`json`"+` fields are marshalled and stored in the table, which will be created if it does not exist. | ||
|
|
||
| The `+"`object`"+` and `+"`array`"+` fields are marshaled as strings. e.g.: | ||
| The `+"`object`"+` and `+"`array`"+` fields are marshaled as strings. for example, |
There was a problem hiding this comment.
Sentence fragment left behind: … marshaled as strings. for example, (lowercase, trailing comma and space, nothing after it).
e.g.: here was introducing the example that follows ("The JSON message: …"). Replacing it with a bare for example, orphans the example — the published page now reads:
The
objectandarrayfields are marshaled as strings. for example,The JSON message:
See the regenerated output at azure_table_storage.adoc#L580-L584. Note the very next paragraph was handled correctly (…properties values. For example:); this one should follow the same pattern, e.g. The \object` and `array` fields are marshaled as strings. For example:` — and drop the trailing whitespace.
CONTRIBUTING.md §1.1.1 (concise, well-organized documentation with configuration examples).
There was a problem hiding this comment.
Fixed. e.g.: was introducing the example blocks, so it is now strings. For example:, matching what rp-connect-docs publishes. The docs-side regeneration caught the same defect.
| "*Memory.* Under `copy-on-write` the whole new-row batch is materialised in memory as a single Arrow record while the batch commits, so a keyed batch's memory scales with its total row bytes. Size keyed batches to stay within the process memory budget rather than making them arbitrarily large.\n" + | ||
| "\n" + | ||
| "*Maintenance.* Because every mutating batch rewrites files and adds a snapshot, a high-churn `copy-on-write` workload accumulates data files and snapshots quickly. Run regular table maintenance: compaction (rewrite / bin-pack data files), snapshot expiry, and orphan-file removal. Orphan-file removal matters specifically for commits that fail *ambiguously* (the catalog may or may not have recorded them): the connector deliberately never deletes those attempts' newly-written data files, because the commit may still land and reference them — they are left for orphan-file removal to reclaim. The connector's own best-effort cleanup covers only commits whose failure was a definitive catalog rejection (and superseded attempts of a retried success). Setting `commit.cleanup_on_failure` to `false` turns that connector-side cleanup off altogether — on every write path — which makes periodic orphan-file removal mandatory rather than merely advisable.\n" + | ||
| "*Maintenance.* Because every mutating batch rewrites files and adds a snapshot, a high-churn `copy-on-write` workload accumulates data files and snapshots quickly. Run regular table maintenance: compaction (rewrite / bin-pack data files), snapshot expiry, and orphan-file removal. Orphan-file removal matters specifically for commits that fail *ambiguously* (the catalog may or may not have recorded them): the connector deliberately never deletes those attempts' newly-written data files, because the commit may still land and reference them, they are left for orphan-file removal to reclaim. The connector's own best-effort cleanup covers only commits whose failure was a definitive catalog rejection (and superseded attempts of a retried success). Setting `commit.cleanup_on_failure` to `false` turns that connector-side cleanup off altogether, on every write path, which makes periodic orphan-file removal mandatory rather than merely advisable.\n" + |
There was a problem hiding this comment.
Two paired em-dashes were each replaced with a comma, turning parentheticals into comma splices that change how the sentence parses.
This line:
…because the commit may still land and reference them**,** they are left for orphan-file removal to reclaim.
and, further down, line 183:
…needs permission to write table properties in addition to writing data**,** a principal without it turns a previously-healthy insert pipeline into a hard failure on first contact after upgrading…
…the set-it-explicitly escape hatch is unavailable by the same rule**,** the
redpanda-connect.prefix carries connector semantics…
In each case two independent clauses are now joined by a bare comma, so the reader has to re-parse — the second one in particular reads as if "writing data" and "a principal" are items in a list. A semicolon, or splitting into two sentences (as was done correctly for commit.cleanup_on_failure at line 430: "…would corrupt the table. Those leftovers are always deferred…"), preserves the original meaning.
These strings are published verbatim as the iceberg output reference; CONTRIBUTING.md §1.1.1 / §1.1.3.
There was a problem hiding this comment.
Checked these against rp-connect-docs main, which publishes exactly this wording for both sentences: ...materialised on disk, and, critically, which query engines can then read the table, is controlled by... and the maintenance paragraph. The commas are the team's reviewed choice for these two, so I have left them rather than re-punctuating published copy. Happy to switch them to parentheses in a follow-up if you would rather, but then the docs side should change with it.
|
The Nothing in this PR changes behavior: the Go changes are doc strings, plus one signature change ( I removed the label so the check stops timing out. If you would rather see the suite run, re-add |
The style pass turned "strings. e.g.:" into "strings. for example,", which starts a sentence in lower case and drops the colon that introduces the JSON example below it. Caught by CodeRabbit on the docs-side regeneration.
| "connector's last checkpoint predates the new incarnation's RESETLOGS_CHANGE#, no log file : \n"+ | ||
| "old or new incarnation: covers that gap. This is not a retention issue, and increasing\n"+ |
There was a problem hiding this comment.
The em dashes here were a matched parenthetical pair (no log file — old or new incarnation — covers that gap), not two separate breaks. Replacing each with a colon leaves the sentence unreadable, and it is the operator-facing text emitted on ORA-00308:
connector's last checkpoint predates the new incarnation's RESETLOGS_CHANGE#, no log file :
old or new incarnation: covers that gap. This is not a retention issue, and increasing
There is also a stray trailing space before \n on line 248. Suggest restoring a single clause, for example ... no log file from either incarnation covers that gap. — which is the wording the oracledb_cdc description already uses for the same failure (input_oracledb_cdc.go).
Two rules apply: CONTRIBUTING.md §1.2.2 ("Provides relevant logging to support troubleshooting") and §3.2.2 (no "difficult-to-diagnose bugs"). Separately, this is an error string, and the commit body for docs: apply house style to connector doc strings states "Code comments, error strings and log messages are left alone" — so either the change is out of scope for that commit or the commit message needs updating (§3.4.3).
There was a problem hiding this comment.
Already reverted in 0f4cd9a - logminer.go is back to main, since those strings are runtime error text rather than published doc strings.
| : the signal table is implicitly added to the publication and excluded from snapshot scans, so listing | ||
| it in both places is rejected at startup. It must have at least these columns: startup validation checks |
There was a problem hiding this comment.
The em dash that was replaced here began a continuation line, so the substitution leaves the description with a line that starts with a bare colon. The generated page shows the result verbatim (postgres_cdc.adoc):
must not also appear in `tables`
: the signal table is implicitly added to the publication and excluded from snapshot scans, so listing
it in both places is rejected at startup.
Beyond reading as a typo, a line beginning with : at the start of an AsciiDoc line is attribute-entry syntax, so this is fragile as well as wrong. The colon belongs at the end of the previous line (... must not also appear in \tables`:`), or the clause should be rejoined into the preceding sentence.
Line 221 compounds it — It must have at least these columns: startup validation checks column names only — where the colon now reads as introducing the column list that actually appears three lines later. Same issue on the list items below: - **type**: should be \VARCHAR` or another string type: the signal typeand- data: should be `TEXT`: a JSON object containing signal parameters` each carry two colons with different meanings.
CONTRIBUTING.md §1.1.1 requires concise, well-organized documentation; a parenthetical em dash needs a comma or parentheses here, not a colon.
| // awsSessionDurationSnapshotNote extends the shared MongoDB `session_duration` | ||
| // description with the part that only applies to this input: the snapshot has | ||
| // no checkpoint, so a credential expiry part-way through costs the whole | ||
| // snapshot. The other MongoDB components have no snapshot phase, which is why | ||
| // this is passed in rather than living on the shared field. | ||
| const awsSessionDurationSnapshotNote = "When using role assumption with this input, credentials are freshly resolved after the initial snapshot completes, so the streaming phase starts with a full session. The snapshot itself must still complete within a single session duration: snapshot progress is not checkpointed, so a credential expiry mid-snapshot restarts the snapshot from scratch after reconnecting. Once the snapshot completes and is fully acknowledged, its position is checkpointed, so later restarts resume the stream without re-running the snapshot. For very large snapshots prefer the ambient credential chain." |
There was a problem hiding this comment.
This const is inserted between isUnresumableTokenError's doc comment and the function itself. Lines 124-154 are that function's godoc (it starts // isUnresumableTokenError reports whether err proves the position stored in the checkpoint cache is permanently unusable...), and because there is no blank line before line 155, the whole block now reads as one comment attached to awsSessionDurationSnapshotNote:
isUnresumableTokenErrorat line 162 is left with no doc comment at all.- The const's doc is prefixed with ~30 lines about change-stream resume-token classification, which has nothing to do with STS session duration.
Move the const and its comment above line 124 (near the other package-level declarations), or below the function, so each declaration keeps its own godoc.
Per the godev patterns, documentation should explain the declaration it sits on; this also falls under CONTRIBUTING.md §3.1.2 (idiomatic, well-structured, self-documenting Go).
There was a problem hiding this comment.
Already fixed in 0f4cd9a - the const now sits with the other package-level declarations, above isUnresumableTokenError's doc comment.
|
|
||
| service.NewStringEnumField(ioFieldMergeStrategy, string(mergeStrategyMOR), string(mergeStrategyCOW)). | ||
| Description("How `upsert` and `delete` are materialised on disk.\n\n* `merge-on-read` (the default) writes Iceberg v2 equality-delete files. Deletes are applied at read time, so writes stay cheap and streaming-friendly, but only catalog-native / Flink-world engines can read the result — engine-backed catalogs such as Snowflake and the Databricks Unity Catalog cannot read equality deletes.\n* `copy-on-write` rewrites whole data files so the table only ever contains plain data files (no delete files), which every engine can read — including Snowflake and Databricks Unity Catalog. It works on version-1 or version-2 tables and never forces the irreversible v1->v2 upgrade. The trade-off is heavy write amplification: each mutating batch rewrites every data file that contains a touched key, so it is a batch / moderate-throughput mode. Sort the table by the identifier key and use large batches so each rewrite touches as few files as possible.\n\nSee the <<merge-strategies,Merge strategies>> section above for the full decision guide, copy-on-write support matrix (column and merge-key types, partitioning, table format), and maintenance guidance."). | ||
| Description("How `upsert` and `delete` are materialised on disk.\n\n* `merge-on-read` (the default) writes Iceberg v2 equality-delete files. Deletes are applied at read time, so writes stay cheap and streaming-friendly, but only catalog-native / Flink-world engines can read the result, engine-backed catalogs such as Snowflake and the Databricks Unity Catalog cannot read equality deletes.\n* `copy-on-write` rewrites whole data files so the table only ever contains plain data files (no delete files), which every engine can read, including Snowflake and Databricks Unity Catalog. It works on version-1 or version-2 tables and never forces the irreversible v1->v2 upgrade. The trade-off is heavy write amplification: each mutating batch rewrites every data file that contains a touched key, so it is a batch / moderate-throughput mode. Sort the table by the identifier key and use large batches so each rewrite touches as few files as possible.\n\nSee the <<merge-strategies,Merge strategies>> section above for the full decision guide, copy-on-write support matrix (column and merge-key types, partitioning, table format), and maintenance guidance."). |
There was a problem hiding this comment.
Three of the em-dash replacements in this file turn a parenthetical or an appositive into a comma splice, and the resulting sentences change how they parse. These are the merge_strategy / commit.cleanup_on_failure reference docs, so the misreading is user-facing.
Line 337 (merge_strategy):
only catalog-native / Flink-world engines can read the result, engine-backed catalogs such as Snowflake and the Databricks Unity Catalog cannot read equality deletes.
Reads on first pass as a list of engines that can read the result — the exact opposite of the intent. A semicolon or a sentence break preserves the contrast.
Line 163 (*Maintenance.*):
because the commit may still land and reference them, they are left for orphan-file removal to reclaim.
The clause after the comma is a new independent sentence; as written, "because the commit may still land and reference them" attaches to the wrong side.
Line 183 (timestamp-encoding):
needs permission to write table properties in addition to writing data, a principal without it turns a previously-healthy insert pipeline into a hard failure
Same problem; "a principal without it turns..." is a new sentence spliced onto the previous one. The second half of that paragraph repeats it: "unavailable by the same rule, the redpanda-connect. prefix carries connector semantics".
In each case a semicolon, a full stop, or parentheses carries the original em dash's meaning; a plain comma does not. CONTRIBUTING.md §1.1.1 / §1.1.3 (documentation should be well-organized and require minimal explanation).
There was a problem hiding this comment.
Fixed in 433c71f, and you were right that these three differ from the earlier two I pushed back on. I checked each against what rp-connect-docs publishes: a semicolon for ...may still land and reference them; they are left... and ...in addition to writing data; a principal without it..., and a sentence break for ...can read the result. Engine-backed catalogs such as.... Applied exactly those.
- Revert logminer.go: those three strings are a runtime error message and its remediation text, not published doc strings, and the paired em dashes in "no log file — old or new incarnation — covers that gap" became two colons that severed the subject from its verb. - Restore the pg_stream signal-table wording rp-connect-docs already publishes. Replacing a dash that opened a soft-wrapped clause left a line starting with a bare colon, and turned a semicolon into a second colon in one sentence. - Move awsSessionDurationSnapshotNote above isUnresumableTokenError's doc comment. It had been inserted inside that comment, so godoc attached the function's text to the const. - Sync three generated pages that docs_gen skipped locally (inputs/kafka, inputs/kafka_franz, inputs/redpanda_common). They now carry the same text as their source, which is what task docs emits. The iceberg comma constructions flagged in review are the wording rp-connect-docs main already publishes for those sentences, so they stay.
| exist in the schema configured via the `schema` field, and must not also appear in `tables` | ||
| — the signal table is implicitly added to the publication and excluded from snapshot scans, so listing | ||
| it in both places is rejected at startup. It must have at least these columns — startup validation checks | ||
| column names only, not types, so a wrong column type (e.g. `data JSONB` instead of `TEXT`) | ||
| : the signal table is implicitly added to the publication and excluded from snapshot scans, so listing | ||
| it in both places is rejected at startup. It must have at least these columns: startup validation checks | ||
| column names only, not types, so a wrong column type (for example, `data JSONB` instead of `TEXT`) |
There was a problem hiding this comment.
This generated page is stale: it no longer matches its source description, so task docs was not re-run after the final fix commit.
Source (fixed in 0f4cd9a), internal/impl/postgresql/input_pg_stream.go#L218-L223:
...must not also appear in `tables`,
since the signal table is implicitly added to the publication and excluded from snapshot scans, so listing
it in both places is rejected at startup. It must have at least these columns; startup validation checks
column names only, not types, so a wrong column type (for example `data JSONB` instead of `TEXT`)
Generated page here still carries the pre-fix text — line 571 begins with a bare : (: the signal table ...), line 572 uses these columns: instead of these columns;, and line 573 uses (for example, instead of (for example . So the published page keeps exactly the malformed sentence the commit set out to remove, and the file disagrees with its generator input.
Per CONTRIBUTING.md §6 — "Run task docs and commit the result: the generated component pages and the internal/plugins/info.csv row. CI fails on stale docs."
Fix: re-run task docs and commit the regenerated postgres_cdc.adoc (worth re-checking the other pages regenerated in 4c716ac for the same drift).
There was a problem hiding this comment.
Fixed in 433c71f: regenerated inputs/postgres_cdc. The previous commit changed the source description but I did not re-run docs_gen after it, which is also what failed the stale-docs check. docs_gen is now a no-op on this branch.
…erate Three em dashes in the iceberg reference became bare commas, which turned a parenthetical and two appositives into comma splices. rp-connect-docs main publishes a semicolon for the first two and a sentence break for the third, so use those. Also regenerates inputs/postgres_cdc, which the previous commit's source fix left stale.
| assembled := acc.Assemble() | ||
| if assembled == nil { | ||
| logDebugf("LOB merge: skipping %s.%s.%s — no fragments accumulated", acc.Schema, acc.Table, acc.Column) | ||
| logDebugf("LOB merge: skipping %s.%s.%s; no fragments accumulated", acc.Schema, acc.Table, acc.Column) |
There was a problem hiding this comment.
These three strings are runtime log messages (logDebugf/logWarnf), not published doc strings, so they fall outside the stated scope of this pass. Commit 614d8e0 says "Only doc strings are touched. Code comments, error strings and log messages are left alone", and commit 0f4cd9a reverted logminer.go for exactly this reason — lob.go looks like it was missed by that revert. Under CONTRIBUTING.md §3.4.3 the subject must accurately describe the change, and docs: apply house style to connector doc strings does not cover log output.
The substitution also degrades the warning at line 235, which now stacks two semicolons in one message:
... has %d candidate events with no PK in WHERE clause; cannot distinguish rows; will synthesize UPDATE
Suggested fix: revert the em dashes on lines 167, 235 and 239 to their original form, matching the logminer.go revert.
| - **type**: should be ` + "`VARCHAR`" + ` or another string type: the signal type (see supported signals below) | ||
| - **data**: should be ` + "`TEXT`" + `: a JSON object containing signal parameters |
There was a problem hiding this comment.
Replacing the em dash with a colon in these two bullets produces a second colon in a line that already uses one as the term/definition separator:
- **type**: should be VARCHAR or another string type: the signal type (see supported signals below)
- **data**: should be TEXT: a JSON object containing signal parameters
The reader can no longer tell which colon introduces the definition, which works against CONTRIBUTING.md §1.1.1 (concise, well-organized reference documentation). This is the same class of defect commit 433c71f fixed in the iceberg reference.
Suggested fix: use a sentence break or parentheses for the trailing clause rather than a second colon — for example should be VARCHAR or another string type (the signal type, see supported signals below) and should be TEXT, a JSON object containing signal parameters.
| "\n" + | ||
| "* Tables created by this output carry `redpanda-connect.timestamp-encoding: spec` from creation.\n" + | ||
| "* For an existing table without the property, the output resolves the encoding automatically on first contact and stamps the result onto the table: if the schema has no no-timezone `timestamp` column, or the table has no data files, it resolves `spec`; otherwise the output inspects one data file's parquet footer and adopts whatever that file already contains (`legacy` for `isAdjustedToUTC=true`). A table that cannot be probed fails the write rather than risk mixing annotations — whether because a footer is unreadable, or because the first 1000 parquet data files probed (in manifest order) carry no no-timezone `timestamp` column at all (possible when the column was added by later schema evolution on a large table); in the latter case the error names the fix: set the property on the table explicitly and the probe never runs. The stamp itself is a `SetProperties` catalog commit, so the principal this output authenticates as needs permission to write table properties in addition to writing data — a principal without it turns a previously-healthy insert pipeline into a hard failure on first contact after upgrading, until the permission is granted or the property is set out-of-band. On a catalog that forbids *all* custom table properties, the set-it-explicitly escape hatch is unavailable by the same rule — the `redpanda-connect.` prefix carries connector semantics and is never silently stripped, so such writes fail loudly rather than risk mixing annotations.\n" + | ||
| "* For an existing table without the property, the output resolves the encoding automatically on first contact and stamps the result onto the table: if the schema has no no-timezone `timestamp` column, or the table has no data files, it resolves `spec`; otherwise the output inspects one data file's parquet footer and adopts whatever that file already contains (`legacy` for `isAdjustedToUTC=true`). A table that cannot be probed fails the write rather than risk mixing annotations: whether because a footer is unreadable, or because the first 1000 parquet data files probed (in manifest order) carry no no-timezone `timestamp` column at all (possible when the column was added by later schema evolution on a large table); in the latter case the error names the fix: set the property on the table explicitly and the probe never runs. The stamp itself is a `SetProperties` catalog commit, so the principal this output authenticates as needs permission to write table properties in addition to writing data; a principal without it turns a previously-healthy insert pipeline into a hard failure on first contact after upgrading, until the permission is granted or the property is set out-of-band. On a catalog that forbids *all* custom table properties, the set-it-explicitly escape hatch is unavailable by the same rule, the `redpanda-connect.` prefix carries connector semantics and is never silently stripped, so such writes fail loudly rather than risk mixing annotations.\n" + |
There was a problem hiding this comment.
Two of the em-dash replacements on this line leave ungrammatical text, the same class of defect commit 433c71f corrected elsewhere in this file:
...the set-it-explicitly escape hatch is unavailable by the same rule, the \redpanda-connect.` prefix carries connector semantics and is never silently stripped...` — the em dash introduced an explanatory clause; a bare comma makes this a comma splice, and the sentence now reads as a list. A semicolon (as used for the analogous case earlier on this line) or a sentence break restores it.A table that cannot be probed fails the write rather than risk mixing annotations: whether because a footer is unreadable, or because...— a colon cannot introducewhether because ...; the original em dash was attaching a parenthetical enumeration. Parentheses or a comma work here.
Per CONTRIBUTING.md §1.1.1, this string is published verbatim as reference documentation, so the regressions ship to users.
1 participant
These strings are published verbatim as reference documentation on docs.redpanda.com. The RPCN docs generator passes prose through unchanged on purpose, so wording and structure have to be right here: correcting them in the docs repo does not hold, and that is what prompted this PR. Fifteen generated partials were corrected in rp-connect-docs one release and the next regeneration reverted every one.
Three commits, separable.
fix(mongodb): scope the session_duration snapshot guidance to the CDC inputAWSIAMAuthFieldis shared by the mongodb cache, input, output, processor andmongodb_cdc, so all five published text describing the CDC snapshot phase: credentials resolved after the snapshot completes, snapshot progress not checkpointed, an expiry mid-snapshot restarting it. Four of the five have no snapshot phase, so the reference described behavior that does not exist for the component being read.The shared field keeps the generic guidance, and a component can pass the part that applies to its own read pattern. Only
mongodb_cdcpasses one.docs(iceberg): give the catalog headings a parent sectionThe
icebergoutput description's headings start at=== Apache Polariswith no==above them, which the docs generator reports as out of sequence every release. It renders correctly today only because the published page supplies its own== Catalog integrationheading, so the description cannot stand on its own anywhere else it is included.docs: house styleThe docs style guide takes commas, parentheses or a sentence break over em dashes, "for example" over "e.g." and "that is" over "i.e.". All 65 reverted lines are fixed, using the wording rp-connect-docs had already settled on wherever it had one, so the published pages do not change beyond the punctuation.
Only doc strings are touched. Code comments, error strings and log messages are left alone.
Verification
gofmtclean; the 25 affected packages build,go vetclean, and their unit tests pass.Not included
Six connector descriptions are long enough that the generator asks for
==sections:inputs/jira,outputs/salesforce_sinkandoutputs/redpanda_migratorhere, plusoutputs/http_server,processors/catchandprocessors/try_catchin redpanda-data/benthos. Adding sections changes the shape of the published pages, so that is an editorial call per page rather than part of a punctuation pass.