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-472] Implement Intermediate Combine #318

Open
wants to merge 17 commits into
base: master
Choose a base branch
from

Conversation

Kangji
Copy link

@Kangji Kangji commented Aug 15, 2021

JIRA: NEMO-472: Fix and Implement Hierarchical Aggregation

Major changes:
[NEMO-472: Implement Hierarchical Aggregation] aims to add additional intermediate accumulation operator in front of final combine operator that accumulates data among physically nearby containers prior to shuffling across WAN, when needed. It is expected that data aggregation among nearby containers will reduce the data size that must be transferred across WAN. To achieve it,

  • Implemented intermediate combine transform
    • Previous Combine.PerKey Transform consisted of 2 steps.
      1. Partial Combine(a.k.a. pre-aggregation): accumulates elements in each containers. Therefore, data transfer across network is not needed in this step.
      2. Final Combine: shuffle all data(hashed by key) and then combine.
    • Additional, and optional step that accumulates the pre-aggregated data partially(only among nearby containers) is implemented and inserted between 1(partial) and 2(final).
    • This new type of transform is only used in intermediate accumulator vertex, which is special type of operator vertex.
  • Added new type of communication channel, Partial Shuffle, which represents data transfer from upstream operator to intermediate accumulator vertex. It resembles shuffle, but the difference is that data shuffle occurs only among physically nearby containers.
  • Implemented compile time optimization pass that inserts intermediate accumulator vertex, which performs hierarchical aggregation prior to shuffle, only when it is expected to be effective.
  • Implemented unit tests.

Minor changes to note:

  • None

Tests for the changes:

  • Tested on my Mac and ubuntu machine

Other comments:

Copy link
Contributor

@taegeonum taegeonum left a comment

Choose a reason for hiding this comment

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

I did my first pass. Overall, looks good to me. Thanks for the work @Kangji !

common/src/main/java/org/apache/nemo/common/ir/IRDAG.java Outdated Show resolved Hide resolved
@@ -45,9 +45,9 @@
* @param <InputT> input type
* @param <OutputT> output type
*/
public final class GBKTransform<K, InputT, OutputT>
public final class CombineTransform<K, InputT, OutputT>
Copy link
Contributor

Choose a reason for hiding this comment

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

Could you explain why the class name is changed?

Copy link
Author

Choose a reason for hiding this comment

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

It's because GroupByKey Transform in Beam semantics can be represented as Combine PerKey Transform, and I thought that this class represents Combine PerKey rather than GroupByKey.

Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I know, GroupByKey Transform is not always be represented as Combine PerKey Transform, so changing the name is confusing to me. For instance, CoGroupByKey is not combining, but it is also represented as GroupByKey as far as I know.

Copy link
Member

Choose a reason for hiding this comment

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

We actually have a separate GroupByKeyTransform. We have renamed the GBKTransform since it actually works as a CombineTransform. It was misnamed in the first place.

@sonarcloud
Copy link

sonarcloud bot commented Aug 30, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@Kangji
Copy link
Author

Kangji commented Aug 30, 2021

@taegeonum Thanks for the review! I've addressed your comments.

Copy link
Contributor

@taegeonum taegeonum left a comment

Choose a reason for hiding this comment

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

@Kangji I did another pass.

import java.util.HashSet;

/**
* List of set of node names to limit the scheduling of the tasks of the vertex to while shuffling.
Copy link
Contributor

Choose a reason for hiding this comment

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

This explanation is not clear to me. Does this property set the destination executor for the output of intermediate vertex?

Copy link
Member

Choose a reason for hiding this comment

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

It limits the sources of the data that each task reads from, depending on where the task is located at. I'll add the explanation.

@@ -45,9 +45,9 @@
* @param <InputT> input type
* @param <OutputT> output type
*/
public final class GBKTransform<K, InputT, OutputT>
public final class CombineTransform<K, InputT, OutputT>
Copy link
Contributor

Choose a reason for hiding this comment

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

As far as I know, GroupByKey Transform is not always be represented as Combine PerKey Transform, so changing the name is confusing to me. For instance, CoGroupByKey is not combining, but it is also represented as GroupByKey as far as I know.

@taegeonum
Copy link
Contributor

@Kangji Any update?

@Kangji
Copy link
Author

Kangji commented Sep 23, 2021

@Kangji Any update?

not yet... :( It has been delayed due to the fall semester, even though i'm trying to do asap. I'll let you know.

@wonook wonook requested a review from taegeonum November 1, 2021 08:13
@sonarcloud
Copy link

sonarcloud bot commented Nov 1, 2021

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

@wonook
Copy link
Member

wonook commented Nov 1, 2021

@taegeonum Can you take a final look?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants