Skip to content
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

[NEMO-429] SWPP TEAM11 Code Smell Fix #276

Merged
merged 26 commits into from Jan 7, 2020
Merged

Conversation

kdh9949
Copy link
Contributor

@kdh9949 kdh9949 commented Dec 2, 2019

JIRA: NEMO-429: TEAM11

Major changes:

  • Fixed code smells (SWPP Code Smell session)

Co-authored-by: wonook wonook@apache.org

Copy link
Member

@wonook wonook left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've left some comments. Please check and apply them on your PR!
Also, it seems like there's a check style error reported. Please run mvn clean install -T1C -DskipTests and see if it works before submitting your final works. Thanks! 😄

@@ -105,7 +105,7 @@ public void testNormalDAG() {

final List<IntegerVertex> topologicalOrder = dag.getTopologicalSort();
assertEquals(topologicalOrder.get(0).getValue(), 4);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe that this was also the line that you were meant to change. Please check and confirm! Thanks :)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@@ -139,9 +139,9 @@ private void fetchDataLazily() {
LOG.error(exception.getMessage());
throw new RuntimeException(exception);
}
});
})
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you merge the lines 142-144 together, so that it becomes something like }))); ?
Other code follow the convention, so it would be better to keep it that way. Thanks!

@@ -710,7 +710,7 @@ public IREdge getEdgeById(final String id) {

@Override
public IREdge getEdgeBetween(final String srcVertexId,
final String dstVertexId) throws IllegalEdgeOperationException {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I can check, this change has to be made on both the common/src/main/java/org/apache/nemo/common/dag/DAG.java and the common/src/main/java/org/apache/nemo/common/dag/DAGInterface.java files, as mentioned here Could you check and confirm and make the appropriate changes there, if applicable? Thanks! 😄

@@ -27,7 +27,7 @@
* Abstract class for optimization passes. All passes basically extends this class.
*/
public abstract class Pass implements Serializable {
private Predicate<IRDAG> condition;
private Predicate<IRDAG> condition; //NOSONAR
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For this, let's leave this line as-is, and remove the Serializable tag on line 29. This makes more sense. Please remove the implements Serializable on line 29, and revert the change on line 30. This will solve the issue.

private final PCollection<KV<Integer, KV<int[], float[]>>> parsedUserData;
private final PCollection<KV<Integer, KV<int[], float[]>>> parsedItemData;
private final PCollection<KV<Integer, KV<int[], float[]>>> parsedUserData; //NOSONAR
private final PCollection<KV<Integer, KV<int[], float[]>>> parsedItemData; //NOSONAR
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For these lines, let's declare the variables transient, instead of putting //nosonar here. In that case, you would have to declare private final transient PCollection... for both lines. Thanks!

@wonook
Copy link
Member

wonook commented Dec 9, 2019

@kdh9949 I've left some comments

@wonook
Copy link
Member

wonook commented Dec 17, 2019

Don't forget to address the comments! @kdh9949 @jangsus1 @geeglegeegle45 @Kojihyung

@wonook wonook merged commit 57e4d59 into apache:master Jan 7, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants