ryucc commented on code in PR #26437:
URL: https://github.com/apache/beam/pull/26437#discussion_r1179506764
##########
runners/samza/src/main/java/org/apache/beam/runners/samza/translation/TranslationContext.java:
##########
@@ -159,12 +182,29 @@ public MessageStream<OpMessage<String>> getDummyStream() {
}
public <OutT> MessageStream<OpMessage<OutT>> getMessageStream(PValue pvalue)
{
+ return getMessageStream(pvalue, true);
+ }
+
+ public <OutT> MessageStream<OpMessage<OutT>> getMessageStream(
Review Comment:
I don't think it's okay. Once we add a boolean flag, it will open the path
two add more later. The number of code paths in one method is 2^n.
I would prefer
```
public <OutT> MessageStream<OpMessage<OutT>>
getMessageStreamWithTransformMetric(PValue pvalue) {
MessageStream<OpMessage<OutT>> stream = getMessageStream(pvalue);
final Config overrideConfig = new
MapConfig(getPipelineOptions().getConfigOverride());
if (doAttachMetricOp(overrideConfig)) {
// add another step if registered for Op Stream
stream.flatMapAsync(
OpAdapter.adapt(
new SamzaInputMetricOp<>(
pvalue.getName(), getTransformFullName(),
beamTransformMetricRegistry),
this));
}
return stream;
}
```
or
```
public <OutT> MessageStream<OpMessage<OutT>>
addTransformMetric(OpMessage<OutT> stream) {
...
}
```
and use the 2 combined.
--
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]
