mynameborat commented on code in PR #25068:
URL: https://github.com/apache/beam/pull/25068#discussion_r1083016937
##########
runners/samza/src/main/java/org/apache/beam/runners/samza/runtime/SamzaDoFnRunners.java:
##########
@@ -316,12 +322,25 @@ public FnDataReceiver<FnOutT> create(String
pCollectionId) {
final TimerReceiverFactory timerReceiverFactory =
new TimerReceiverFactory(stageBundleFactory,
this::timerDataConsumer, windowCoder);
+ Map<String, String> transformFullNameToUniqueName =
+ pTransformNodes.stream()
+ .collect(
+ Collectors.toMap(
+ pTransformNode -> pTransformNode.getId(),
+ pTransformNode ->
pTransformNode.getTransform().getUniqueName()));
Review Comment:
Its good practice to keep constructor light weight that said, it doesn't
have to always have assignments within it. Some initialization logic is okay.
Other benefits doing so,
- it helps building invariants that this field isn't going to change upon
construction
- handle to pTransformNode is unnecessary and can potentially be meddled
with which will make this piece of non deterministic breaking the assumption
that `transformFullNameToUniqueName` changes.
- Ideally, you would want only the names to be dependency injected if you
don't need the `pTransformNode` at all.
The code you pasted above is mostly blueprint and not performing any
computation per-say. local variables are introduced for readability and might
as well replace them with the getters of the already existing instance
variables.
--
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]
