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.

Sign up for GitHub

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

Jump to bottom

Fix resource leaks and suppress an incorrect LGTM alert #8589

Merged
merged 2 commits into from Oct 10, 2019
Merged

Fix resource leaks and suppress an incorrect LGTM alert #8589

merged 2 commits into from Oct 10, 2019

Conversation

asdf2014
Copy link
Member

Description

Fix resource leaks and suppress an incorrect LGTM alert

This PR has:

  • been self-reviewed.
  • added documentation for new or modified features or behaviors.
  • added Javadocs for most classes and all non-trivial methods. Linked related entities via Javadoc links.
  • added or updated version, license, or notice information in licenses.yaml
  • added comments explaining the "why" and the intent of the code wherever would not be obvious for an unfamiliar reader.
  • added unit tests or modified existing tests to cover new code paths.
  • added integration tests.
  • been tested in a test Druid cluster.

leventov
leventov reviewed Oct 10, 2019
View reviewed changes
indexing-hadoop/src/main/java/org/apache/druid/indexer/JobHelper.java Outdated
@@ -364,9 +365,9 @@ public static void ensurePaths(HadoopDruidIndexerConfig config)
public static void writeJobIdToFile(String hadoopJobIdFileName, String hadoopJobId)
{
if (hadoopJobId != null && hadoopJobIdFileName != null) {
try {
try (final OutputStream out = java.nio.file.Files.newOutputStream(Paths.get(hadoopJobIdFileName))) {
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 replace the sole use of Guava's Files in this class with java.nio.file.Files.copy() and then avoid fully-qualified class name?

Copy link
Member Author

Choose a reason for hiding this comment

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

Done.

leventov
leventov approved these changes Oct 10, 2019
View reviewed changes
Copy link
Member

@leventov leventov left a comment

Choose a reason for hiding this comment

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

Thanks!

@leventov leventov merged commit bba262a into apache:master Oct 10, 2019
@asdf2014 asdf2014 deleted the resource_leak branch October 11, 2019 02:11
@jon-wei jon-wei added this to the 0.17.0 milestone Dec 17, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@leventov leventov leventov approved these changes

Assignees
No one assigned
Labels
Improvement
Projects
None yet
Milestone
0.17.0
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@asdf2014 @leventov @jon-wei