Skip to content

arc: add output plugin for Arc columnar database#4265

Merged
josephwoodward merged 1 commit intomainfrom
mmt/arc-cloud-bundle
Apr 16, 2026
Merged

arc: add output plugin for Arc columnar database#4265
josephwoodward merged 1 commit intomainfrom
mmt/arc-cloud-bundle

Conversation

@mmatczuk
Copy link
Copy Markdown
Contributor

No description provided.

Comment thread internal/impl/arc/output.go Outdated
@claude
Copy link
Copy Markdown

claude bot commented Apr 16, 2026

Commits
LGTM

Review
Clean new component implementation. The arc output follows project conventions for registration, config spec, bundle imports, public wrapper, info.csv metadata, license headers, error handling, and context propagation. One naming convention violation in field name constants.

  1. Field name constants lack component prefix (internal/impl/arc/output.go#L47-L57): Constants like fieldToken, fieldDatabase, etc. should follow the project convention <componentAbbrev>Field<Name> (e.g., aoFieldToken, aoFieldDatabase), as documented in the project Go patterns and used consistently across all other components.

@mmatczuk mmatczuk force-pushed the mmt/arc-cloud-bundle branch from 6d99dc9 to f638a2d Compare April 16, 2026 10:16
Comment on lines +106 to +112
batch := service.MessageBatch{
service.NewMessage([]byte(`{"sensor":"temp-1","value":22.5}`)),
service.NewMessage([]byte(`{"sensor":"temp-2","value":23.1}`)),
}

require.NoError(t, out.WriteBatch(t.Context(), batch))

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: rowCount and err in the require.Eventually format args are evaluated at call time — before the polling loop runs — so they will always be 0 and nil (their zero values). On failure the message will read "expected N rows, got 0 (last err: <nil>)" regardless of what actually happened, making CI failures hard to debug.

Fix: format the message inside the condition closure instead. For example, capture a lastMsg string variable and update it each iteration, then use require.Eventuallyf or move the assertion outside the Eventually and check the final values after the loop.

@claude
Copy link
Copy Markdown

claude bot commented Apr 16, 2026

Commits
LGTM

Review
New arc output component for writing to an Arc columnar database via msgpack ingestion. Implementation follows project patterns: proper init() registration with MustRegisterBatchOutput, field name constants with ao prefix, ParsedConfig extraction, *service.Resources logger pattern, Apache 2.0 headers, public wrapper, community + cloud bundle imports, and info.csv entry. Unit and integration tests cover columnar/row formats, compression, auth, error handling, and timestamp conversion.

  1. Integration test requireArcQueryRowCount produces misleading error messages on failure (integration_test.go#L106-L112): rowCount and err passed as require.Eventually format args are captured at call time (zero values), not when the assertion fails. On timeout the message will always say "expected N rows, got 0 (last err: <nil>)" regardless of the actual last observed state.

Co-Authored-By: Michał Matczuk <michal.matczuk@redpanda.com>
@mmatczuk mmatczuk force-pushed the mmt/arc-cloud-bundle branch from f638a2d to 4df201e Compare April 16, 2026 10:35
@claude
Copy link
Copy Markdown

claude bot commented Apr 16, 2026

Commits
LGTM

Review
New arc output component for writing to an Arc columnar database via MessagePack ingestion. Adds implementation, unit tests, integration tests, public wrapper, bundle registration (community + cloud), info.csv metadata, and autogenerated docs. Code follows project patterns correctly: MustRegisterBatchOutput in init(), field name constants with ao prefix, proper error wrapping with gerund form and %w, context propagation through component methods, safe sync.Pool usage for compression writers, header injection prevention on token and database name, and idempotent Close.

LGTM

@xe-nvdk
Copy link
Copy Markdown
Contributor

xe-nvdk commented Apr 16, 2026

Why doing this? Hard to track contributors, doesn't feel like a good practice. Can you clarify?

@mmatczuk
Copy link
Copy Markdown
Contributor Author

@xe-nvdk to streamline things on your end like failing CI and to be able to include it in a release. Cheers.

}

rec.columns["time"] = append(rec.columns["time"], ts)
for k, v := range dataMap {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we be concerned with map's random ordering here?

rec.columns["time"] = append(rec.columns["time"], ts)
for k, v := range dataMap {
if k == o.timestampField || k == "time" {
continue
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should we document/log that any message field named "time" is dropped silently?

default:
return fmt.Errorf("unsupported format: %s", o.format)
}
if err != nil {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given we return on err here, would it not be better returning earlier (within the case)?

return nil
}

var payload any
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion:

var (
    payload any
    err error
)


data, err := msg.AsStructuredMut()
if err != nil {
return nil, fmt.Errorf("message %d: %w", i, err)
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure how helpful this error message prefix is (message %d), Is there a clearer prefix we could use?

@josephwoodward
Copy link
Copy Markdown
Contributor

None of these comments are blockers and can be addressed post merge.

@josephwoodward josephwoodward merged commit 834a001 into main Apr 16, 2026
7 checks passed
@josephwoodward josephwoodward deleted the mmt/arc-cloud-bundle branch April 16, 2026 16:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants