Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

[CAY-1207] Decouple Pregel framework from pagerank app #1212

Merged
merged 15 commits into from
Jul 21, 2017
Merged

Conversation

JunhoeKim
Copy link
Contributor

@JunhoeKim JunhoeKim commented Jul 20, 2017

Closes #1207

Using PregelConfiguration, users can launch the Pregel according to the desired setting.
For example, PagerankET launches pagerank app on Pregel.

@JunhoeKim JunhoeKim requested a review from wynot12 July 20, 2017 09:08
Copy link
Contributor

@wynot12 wynot12 left a comment

Choose a reason for hiding this comment

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

Would you take a look at my comments and commits?
Thanks!


@Override
public PregelConfiguration build() {
return new PregelConfiguration(this);
Copy link
Contributor

Choose a reason for hiding this comment

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

Passing each ingredient looks better than a builder.
Then we don't need get methods in Builder class.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'll update it by myself.

@wynot12 wynot12 added the Pregel label Jul 20, 2017
@@ -105,15 +108,15 @@ public Builder setMessageCodecClass(final Class<? extends Codec> messageCodecCla
return this;
}

public Builder setMessageUpdateFunctionClass(final Class<? extends UpdateFunction> updateFunctionClass) {
this.messageUpdateFunctionClass = updateFunctionClass;
public Builder addParameterClass(final Class<? extends Name<?>> parameterClass) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It's for future usage, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

#1178 may use this feature, right?

@wynot12
Copy link
Contributor

wynot12 commented Jul 21, 2017

LGTM!
I'll merge it after running an example.
Thanks :)

@wynot12 wynot12 merged commit 665bca9 into master Jul 21, 2017
@wynot12 wynot12 deleted the pregel-extend branch July 21, 2017 07:32
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants