Add /dev/null id option and surface resume token update interval.#315
Add /dev/null id option and surface resume token update interval.#315adiom-mark merged 1 commit intomainfrom
Conversation
WalkthroughThe null connector gains an id field and exposes it via GetInfo. NewConn now requires an id string as the first parameter; all call sites and CLI wiring are updated, including a new --id flag. A hidden cdc-resume-token-interval duration flag is added. Mongo connector removes a debug log. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant User
participant CLI as CLI Options
participant Factory as Connector Factory
participant Null as Null Connector
participant RPC as GetInfo RPC
User->>CLI: Provide flags (--id, --log-json, --sleep, --sleep-jitter,<br/>--cdc-resume-token-interval)
note right of CLI: New flag: cdc-resume-token-interval (duration)
CLI->>Factory: Build config (id, logJson, sleep, sleepJitter)
Factory->>Null: NewConn(id, logJson, sleep, sleepJitter)
activate Null
Null-->>Factory: conn{ id: string, ... }
deactivate Null
User->>RPC: GetInfo()
RPC->>Null: GetInfo
Note over Null: Returns Id from internal field
Null-->>RPC: GetInfoResponse{ Id, ... }
RPC-->>User: Response with Id
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (7)
internal/app/options/flags.go (1)
147-151: Clarify duration flag semantics (units, default).Add a Usage to indicate expected format (Go duration) and that 0 disables periodic updates.
- altsrc.NewDurationFlag(&cli.DurationFlag{ - Name: "cdc-resume-token-interval", - Required: false, - Hidden: true, - }), + altsrc.NewDurationFlag(&cli.DurationFlag{ + Name: "cdc-resume-token-interval", + Usage: "Interval to persist CDC resume token (e.g., 30s, 1m). Zero disables.", + Required: false, + Hidden: true, + }),Please confirm that existing configs (if any) already use Go duration strings; otherwise, we may need a migration note.
connectors/mongo/conn.go (1)
564-564: Remove commented-out debug or reintroduce with proper level gating.Leaving commented logs adds noise. Prefer deletion or a structured debug gated by log level.
- // slog.Debug(fmt.Sprintf("Converting change stream event %v", change))internal/app/options/options.go (1)
69-69: Duration parsing change may break older configs using bare integers.cli.Duration requires unit-suffixed values (e.g., "30s"). If existing YAML/env used plain integers for seconds, they'll now parse as 0.
If back-compat is required, consider a hidden legacy seconds flag as fallback or accept numeric strings by custom parsing before defaulting to c.Duration("cdc-resume-token-interval").
cmd/grpcexample/main.go (1)
35-35: Use a non-empty example id for clarity.Demonstrates intent and surfaces the new field in GetInfo.
- nullConn := null.NewConn("", false, 0, 0) + nullConn := null.NewConn("grpcexample", false, 0, 0)connectors/null/connector_test.go (1)
22-23: Add a focused test asserting id propagation via GetInfo.Covers the new constructor arg and API surface.
Example test (new file or appended):
func TestNullConnector_GetInfo_Id(t *testing.T) { h := NewConn("test-id", false, 0, 0) _, handler := adiomv1connect.NewConnectorServiceHandler(h) srv, err := memhttp.New(handler) if err != nil { t.Fatal(err) } client := adiomv1connect.NewConnectorServiceClient(srv.Client(), srv.URL()) resp, err := client.GetInfo(context.Background(), connect.NewRequest(&adiomv1.GetInfoRequest{})) if err != nil { t.Fatal(err) } if got := resp.Msg.GetId(); got != "test-id" { t.Fatalf("Id = %q, want %q", got, "test-id") } }Also applies to: 29-30
internal/app/options/connectorflags.go (1)
291-294: Allow setting id via env var; confirm empty id is acceptable downstream.Expose DSYNC_CONNECTOR_ID for non-interactive use. Also verify that an empty id is safe for all consumers of GetInfo().Id.
&cli.StringFlag{ Name: "id", Usage: "A fixed id for the connector", + EnvVars: []string{"DSYNC_CONNECTOR_ID"}, }, ... - return null.NewConn(c.String("id"), c.Bool("log-json"), c.Duration("sleep"), c.Duration("sleep-jitter")), nil + return null.NewConn(c.String("id"), c.Bool("log-json"), c.Duration("sleep"), c.Duration("sleep-jitter")), nilAlso applies to: 296-296
connectors/null/connector.go (1)
37-47: GetInfo now exposes Id — OK; consider stable ordering of SupportedDataTypesExposing
Id: c.idis fine. Optional: buildallin a stable, deterministic order (e.g., sort by enum value) to avoid nondeterministic diffs/logs across runs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
cmd/grpcexample/main.go(1 hunks)connectors/mongo/conn.go(1 hunks)connectors/null/connector.go(3 hunks)connectors/null/connector_test.go(1 hunks)internal/app/options/connectorflags.go(1 hunks)internal/app/options/flags.go(1 hunks)internal/app/options/options.go(1 hunks)
🧰 Additional context used
🧬 Code graph analysis (4)
internal/app/options/connectorflags.go (1)
connectors/null/connector.go (1)
NewConn(148-150)
cmd/grpcexample/main.go (1)
connectors/null/connector.go (1)
NewConn(148-150)
connectors/null/connector_test.go (3)
connectors/common/base.go (1)
NewLocalConnector(884-896)connectors/null/connector.go (1)
NewConn(148-150)pkg/test/connector.go (2)
NewConnectorTestSuite(39-41)ClientFromHandler(30-37)
connectors/null/connector.go (5)
connectors/mongo/conn.go (1)
NewConn(933-941)connectors/cosmos/conn.go (1)
NewConn(515-552)connectors/testconn/connector.go (1)
NewConn(388-394)connectors/dynamodb/conn.go (1)
NewConn(381-403)connectors/random/connector.go (1)
NewConn(124-138)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build
🔇 Additional comments (2)
connectors/null/connector.go (2)
24-28: Adding id field to conn looks goodNo functional concerns. This enables plumbing the connector id cleanly.
148-150: Prefer keyed struct literal and tighten signatureReplace the unkeyed composite literal with a keyed one and combine identical time.Duration parameters; call sites already pass four args so no call-site changes are needed.
File: connectors/null/connector.go (around lines 148–150)
-func NewConn(id string, logJson bool, sleep time.Duration, sleepJitter time.Duration) adiomv1connect.ConnectorServiceHandler { - return &conn{id, logJson, sleep, sleepJitter} +func NewConn(id string, logJson bool, sleep, sleepJitter time.Duration) adiomv1connect.ConnectorServiceHandler { + return &conn{ + id: id, + logJson: logJson, + sleep: sleep, + sleepJitter: sleepJitter, + } }
1 participant
Summary by CodeRabbit
New Features
Chores