[NEMO-429] SWPP TEAM3 Code Smell Fix #265
Conversation
There was a problem hiding this comment.
Thanks for the PR. I left a comment.
Regarding the two transient issues assigned to your team:
(1) ReduceTransform
I took a look, and it appears that there's no good reason for private T result to be a class variable. Can you move the variable into the onData() method, which is the only place where the variable is used?
(2) ReduceByKeyTransform
This one is tricky. Can you add a TODO that references the following JIRA issue?
https://issues.apache.org/jira/browse/NEMO-431
Revert other lines
make T as a variable of onData()
Add a TODO comment
|
@johnyangk I reflected your review. Thank you! |
There was a problem hiding this comment.
I took a more close look at ReduceTransform, and it seems that the result variable does need to be shared across multiple onData() calls, since it sort of accumulates the resulting value. Hence, I take back what I said. result should be kept as a class variable. Sorry for the confusion.
Everything else looks good to me.
...d/spark/src/main/java/org/apache/nemo/compiler/frontend/spark/transform/ReduceTransform.java
Show resolved
Hide resolved
...d/spark/src/main/java/org/apache/nemo/compiler/frontend/spark/transform/ReduceTransform.java
Show resolved
Hide resolved
...d/spark/src/main/java/org/apache/nemo/compiler/frontend/spark/transform/ReduceTransform.java
Outdated
Show resolved
Hide resolved
...d/spark/src/main/java/org/apache/nemo/compiler/frontend/spark/transform/ReduceTransform.java
Show resolved
Hide resolved
Reflected the review
|
@johnyangk I reflected it, thank you! |
JIRA: NEMO-429: SWPP TEAM 3 Code Smell Fix
Major changes:
Notes:
transientprefix is good solution, so we didn't fix about it. Please let us know if it should be fixed.By Team3
P.S. One of our members was absent today.