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
[FLINK-6358] [gelly] Write job details for Gelly examples #4170
[FLINK-6358] [gelly] Write job details for Gelly examples #4170
Conversation
@@ -103,6 +113,18 @@ | |||
.addClass(PageRank.class) | |||
.addClass(TriangleListing.class); | |||
|
|||
private ParameterTool parameters; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
set parameters
in constructor, you will avoid having a null value
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@@ -103,6 +113,18 @@ | |||
.addClass(PageRank.class) | |||
.addClass(TriangleListing.class); | |||
|
|||
private ParameterTool parameters; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private final
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@@ -103,6 +113,18 @@ | |||
.addClass(PageRank.class) | |||
.addClass(TriangleListing.class); | |||
|
|||
private ParameterTool parameters; | |||
|
|||
private BooleanParameter disableObjectReuse = new BooleanParameter(this, "__disable_object_reuse"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private final
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
|
||
private BooleanParameter disableObjectReuse = new BooleanParameter(this, "__disable_object_reuse"); | ||
|
||
private StringParameter jobDetails = new StringParameter(this, "__write_job_details") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
private final
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
|
||
@Override | ||
public String getName() { | ||
return this.getClass().getSimpleName(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is a Runner
good value to be returned to the user? Is this used anywhere?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is currently unused.
@@ -70,7 +79,8 @@ | |||
* <p>Algorithms must explicitly support each type of output via implementation of | |||
* interfaces. This is scalable as the number of outputs is small and finite. | |||
*/ | |||
public class Runner { | |||
public class Runner | |||
extends ParameterizedBase { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do you extend ParametrizedBase
? Isn't it intended as a base class for parameters? Do you use Runner
in such context?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Parameter
/ SimpleParameter
are the interface / base class for parameters whereas Parameterized
/ ParameterizedBase
are the interface / base class for components containing the parameters.
@@ -305,6 +334,41 @@ public static void main(String[] args) throws Exception { | |||
default: | |||
throw new ProgramParametrizationException("Unknown output type: " + outputName); | |||
} | |||
|
|||
// ---------------------------------------------------------------------------------------- | |||
// Write job details |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would nicer to extract this if condition to writeJobDetails
method and delete this comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
|
||
private BooleanParameter disableObjectReuse = new BooleanParameter(this, "__disable_object_reuse"); | ||
|
||
private StringParameter jobDetails = new StringParameter(this, "__write_job_details") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
rename parameter to __job_details_path
? __write_job_details_to_path
?
and rename field to jobDetailsPath
.
Now it is unclear what this variable represents without looking at the usages.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed to jobDetailsPath
and __job_details_path
. Plan to add descriptions in the future.
Add an option to write job details to a file in JSON format. Job details include: job ID, runtime, parameters with values, and accumulators with values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the review @pnowojski. I believe I have addressed all of your comments.
@@ -70,7 +79,8 @@ | |||
* <p>Algorithms must explicitly support each type of output via implementation of | |||
* interfaces. This is scalable as the number of outputs is small and finite. | |||
*/ | |||
public class Runner { | |||
public class Runner | |||
extends ParameterizedBase { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Parameter
/ SimpleParameter
are the interface / base class for parameters whereas Parameterized
/ ParameterizedBase
are the interface / base class for components containing the parameters.
@@ -103,6 +113,18 @@ | |||
.addClass(PageRank.class) | |||
.addClass(TriangleListing.class); | |||
|
|||
private ParameterTool parameters; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@@ -103,6 +113,18 @@ | |||
.addClass(PageRank.class) | |||
.addClass(TriangleListing.class); | |||
|
|||
private ParameterTool parameters; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@@ -103,6 +113,18 @@ | |||
.addClass(PageRank.class) | |||
.addClass(TriangleListing.class); | |||
|
|||
private ParameterTool parameters; | |||
|
|||
private BooleanParameter disableObjectReuse = new BooleanParameter(this, "__disable_object_reuse"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
|
||
private BooleanParameter disableObjectReuse = new BooleanParameter(this, "__disable_object_reuse"); | ||
|
||
private StringParameter jobDetails = new StringParameter(this, "__write_job_details") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
|
||
@Override | ||
public String getName() { | ||
return this.getClass().getSimpleName(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It is currently unused.
|
||
private BooleanParameter disableObjectReuse = new BooleanParameter(this, "__disable_object_reuse"); | ||
|
||
private StringParameter jobDetails = new StringParameter(this, "__write_job_details") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Renamed to jobDetailsPath
and __job_details_path
. Plan to add descriptions in the future.
@@ -305,6 +334,41 @@ public static void main(String[] args) throws Exception { | |||
default: | |||
throw new ProgramParametrizationException("Unknown output type: " + outputName); | |||
} | |||
|
|||
// ---------------------------------------------------------------------------------------- | |||
// Write job details |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
f7a89bc
to
1dc21f9
Compare
Add an option to write job details to a file in JSON format. Job details include: job ID, runtime, parameters with values, and accumulators with values.