-
Notifications
You must be signed in to change notification settings - Fork 926
Add batching implementation for redis_hash output #3772
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -31,6 +31,7 @@ const ( | |
| hoFieldWalkMetadata = "walk_metadata" | ||
| hoFieldWalkJSON = "walk_json_object" | ||
| hoFieldFields = "fields" | ||
| hoFieldBatching = "batching" | ||
| ) | ||
|
|
||
| func redisHashOutputConfig() *service.ConfigSpec { | ||
|
|
@@ -80,13 +81,17 @@ Where latter stages will overwrite matching field names of a former stage.`+serv | |
| Description("A map of key/value pairs to set as hash fields."). | ||
| Default(map[string]any{}), | ||
| service.NewOutputMaxInFlightField(), | ||
| service.NewBatchPolicyField(loFieldBatching), | ||
| ) | ||
| } | ||
|
|
||
| func init() { | ||
| service.MustRegisterOutput( | ||
| service.MustRegisterBatchOutput( | ||
| "redis_hash", redisHashOutputConfig(), | ||
| func(conf *service.ParsedConfig, mgr *service.Resources) (out service.Output, maxInFlight int, err error) { | ||
| func(conf *service.ParsedConfig, mgr *service.Resources) (out service.BatchOutput, batchPol service.BatchPolicy, maxInFlight int, err error) { | ||
| if batchPol, err = conf.FieldBatchPolicy(loFieldBatching); err != nil { | ||
| return | ||
| } | ||
| if maxInFlight, err = conf.FieldMaxInFlight(); err != nil { | ||
| return | ||
| } | ||
|
|
@@ -168,20 +173,14 @@ func walkForHashFields(msg *service.Message, fields map[string]any) error { | |
| return nil | ||
| } | ||
|
|
||
| func (r *redisHashWriter) Write(ctx context.Context, msg *service.Message) error { | ||
| r.connMut.RLock() | ||
| client := r.client | ||
| r.connMut.RUnlock() | ||
|
|
||
| if client == nil { | ||
| return service.ErrNotConnected | ||
| } | ||
|
|
||
| func (r *redisHashWriter) buildMessage(msg *service.Message) (string, map[string]any, error) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Bug: The batch path should use Fix: either make |
||
| key, err := r.key.TryString(msg) | ||
| if err != nil { | ||
| return fmt.Errorf("key interpolation error: %w", err) | ||
| return "", nil, fmt.Errorf("key interpolation error: %w", err) | ||
| } | ||
|
|
||
| fields := map[string]any{} | ||
|
|
||
| if r.walkMetadata { | ||
| _ = msg.MetaWalkMut(func(k string, v any) error { | ||
| fields[k] = v | ||
|
|
@@ -190,21 +189,70 @@ func (r *redisHashWriter) Write(ctx context.Context, msg *service.Message) error | |
| } | ||
| if r.walkJSON { | ||
| if err := walkForHashFields(msg, fields); err != nil { | ||
| err = fmt.Errorf("failed to walk JSON object: %v", err) | ||
| r.log.Errorf("HSET error: %v\n", err) | ||
| return err | ||
| return "", nil, fmt.Errorf("HSET error: failed to walk JSON object: %v", err) | ||
| } | ||
| } | ||
| for k, v := range r.fields { | ||
| if fields[k], err = v.TryString(msg); err != nil { | ||
| return fmt.Errorf("field %v interpolation error: %w", k, err) | ||
| return "", nil, fmt.Errorf("field %v interpolation error: %w", k, err) | ||
| } | ||
| } | ||
| if err := client.HSet(ctx, key, fields).Err(); err != nil { | ||
| return key, fields, nil | ||
| } | ||
|
|
||
| func (r *redisHashWriter) WriteBatch(ctx context.Context, batch service.MessageBatch) error { | ||
| r.connMut.RLock() | ||
| client := r.client | ||
| r.connMut.RUnlock() | ||
|
|
||
| if client == nil { | ||
| return service.ErrNotConnected | ||
| } | ||
|
|
||
| if len(batch) == 1 { | ||
| key, fields, err := r.buildMessage(batch[0]) | ||
| if err != nil { | ||
| err = fmt.Errorf("failed to create message: %v", err) | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Rule: "Use |
||
| return err | ||
| } | ||
| if err := client.HSet(ctx, key, fields).Err(); err != nil { | ||
| _ = r.disconnect() | ||
| r.log.Errorf("Error from redis: %v\n", err) | ||
| return service.ErrNotConnected | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
| pipe := client.Pipeline() | ||
|
|
||
| for i := range batch { | ||
| key, fields, err := r.buildMessage(batch[i]) | ||
| if err != nil { | ||
| err = fmt.Errorf("failed to create message: %v", err) | ||
| return err | ||
| } | ||
| _ = pipe.HSet(ctx, key, fields) | ||
| } | ||
|
|
||
| cmders, err := pipe.Exec(ctx) | ||
| if err != nil { | ||
| _ = r.disconnect() | ||
| r.log.Errorf("Error from redis: %v\n", err) | ||
| return service.ErrNotConnected | ||
| } | ||
|
|
||
| var batchErr *service.BatchError | ||
| for i, res := range cmders { | ||
| if res.Err() != nil { | ||
| if batchErr == nil { | ||
| batchErr = service.NewBatchError(batch, res.Err()) | ||
| } | ||
| batchErr.Failed(i, res.Err()) | ||
| } | ||
| } | ||
| if batchErr != nil { | ||
| return batchErr | ||
| } | ||
| return nil | ||
| } | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hoFieldBatchingis defined at line 34 butloFieldBatching(from the redis list output) is used here and at line 92. Both resolve to"batching"so it works at runtime, but this should use the hash output's own constanthoFieldBatching.Rule: field names must use the component-prefix convention
<componentAbbrev>Field<Name>— godev agent.