-
Notifications
You must be signed in to change notification settings - Fork 440
TEZ-4617 prevent resource leaks for Closeables #402
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
base: master
Are you sure you want to change the base?
Conversation
|
💔 -1 overall
This message was automatically generated. |
| @SuppressWarnings("deprecation") | ||
| public int run(String[] args) throws Exception { | ||
| Configuration conf = getConf(); | ||
| JobClient client = new JobClient(conf); |
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.
would it make sense to refactor the method to use the JobClient to prevent many indentation, like:
try (JobClient client = new JobClient(conf))
run(client)
}
private run (JobClient client) {
}
| Class.forName(args[++i]).asSubclass(OutputFormat.class); | ||
| } else { | ||
| otherArgs.add(args[i]); | ||
| try (JobClient client = new JobClient(conf)) { |
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.
same here
| conf.setLong(BYTES_PER_MAP, totalBytesToWrite); | ||
| } | ||
| conf.setInt(MRJobConfig.NUM_MAPS, numMaps); | ||
| try (JobClient client = new JobClient(conf)) { |
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.
same here
| pw.write("<script type='text/javascript'>setTimeout(function() { " + | ||
| "window.location.replace('" + historyUrl + "');" + | ||
| "}, 0); </script>"); | ||
| try(PrintWriter pw = writer()) { |
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.
nit: space after try
| @Override | ||
| public void render() { | ||
| response().setContentType(MimeType.HTML); | ||
| PrintWriter pw = writer(); |
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.
what about a separate method like:
try (PrintWriter pw = writer()) {
render(pw);
}
|
thanks for this patch @dk2k, left some comments |
No description provided.