Explicitly release resources in LogFileReader and TestHoodieClientBase. Fixes Memory allocation errors #463
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@n3nash @vinothchandar : PR to make master green. Unit tests passes with this PR in Travis CI |
bvaradar
changed the title
n3nash
reviewed
Sep 20, 2018
hoodie-common/src/main/java/com/uber/hoodie/common/table/log/HoodieLogFormatReader.java
Show resolved
Hide resolved
hoodie-client/src/test/java/com/uber/hoodie/TestHoodieClientBase.java
Outdated
Show resolved
Hide resolved
hoodie-common/src/main/java/com/uber/hoodie/common/table/log/HoodieLogFormatReader.java
Show resolved
Hide resolved
hoodie-common/src/main/java/com/uber/hoodie/common/table/log/HoodieLogFormatReader.java
Show resolved
Hide resolved
bvaradar
force-pushed
the
fix_failure
branch
from
75e39ef
to
0b6f175
Compare
|
Thanks @n3nash @vinothchandar for doing a quick review. I have addressed/Replied-to your review comments. Please take a look. |
|
LGTM |
bvaradar
force-pushed
the
fix_failure
branch
from
0b6f175
to
4bd15b0
Compare
bvaradar
commented
Sep 20, 2018
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Some Context HERE:
Issue : #462
There were 2 issues here :
Our maven surefire configs causes one JVM forked process to run all tests in a single module serially.
Looking at different test logs where memory allocation failed, it was clear the issue is happening only when running tests in hoodie-client.
Did some divide-n-conquer in localizing the tests by looking at Yourkit profile results and opening some dummy PRs which selectively disables some suspects ( https://github.com/bvaradar/hudi/pulls ). Was able to localize by disabling some of the tests which allowed the tests to pass in TravisCI.
The base-test class TestHoodieClientBase which sets up each HoodieClient tests and TestMergeOnReadTable was leaking resources. Part of the PR fixes this.
This is caused by unchecked close of inputStream in HoodieLogReader. Also, found that we have not properly handled closing of HoodieLogFormatReader (which encapsulates multiple log file reading). I had updated the code to explicitly call close() at different LogFileReader abstractions and also made all the unit-tests properly close the log-file readers they were using. This fixed the "FileSystem closed" exceptions we were seeing in unit-tests.