jonathansudhakar1 commented on issue #23550:
URL: https://github.com/apache/beam/issues/23550#issuecomment-6610627288
@lostluck, I'd like to pick this up. Since the issue is a few years old, I
wanted to check the approach with you first.
It still reproduces on master:
- `register.Combiner1[int](c)`, where `c` has `MergeAccumulators(ctx
context.Context, a, b int) int`, panics with `Failed to optimize
MergeAccumulators for combiner &{}. Failed to infer types`.
- `Combiner3` panics the same way for a combiner that takes a ctx in every
method.
`graph`'s CombineFn validation already accepts an optional leading
`context.Context` on all four methods, so only `register` rejects it.
Plan:
- **Template:** in `sdks/go/pkg/beam/register/register.tmpl`, add ctx-first
variants of the extraction interfaces: `createAccumulator1x1/1x2`,
`addInput3x1/3x2`, `mergeAccumulators3x1/3x2` and `extractOutput2x1/2x2`.
- **`CombinerN`:** add a branch for each variant that registers the matching
caller for the `func(context.Context, ...)` type and builds the method wrapper,
the same way the existing branches do.
- **Regenerate** `register.go` with the `go generate` directives in `doc.go`.
- **Compatibility:** combiners without a ctx behave exactly as today. The
"Failed to optimize" panics stay for signatures that are still unsupported.
- **Tests** in `register_test.go`:
- For `Combiner1/2/3`: combiners with a ctx in every method, with and
without `error` returns. Assert that `reflectx.WrapMethods` returns working
wrappers for all four methods.
- A `ptest` pipeline that runs `beam.Combine` on Prism with a registered
ctx-taking combiner.
Does that match what you had in mind? Or would you rather fix it outside the
template, for example by skipping optimization for ctx methods instead of
panicking?
.take-issue
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
