-
Notifications
You must be signed in to change notification settings - Fork 703
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
[CARBONDATA-3051] Fix bugs in unclosed streams #2871
Conversation
Failed to run CarbonReaderTests in windows env. This is due to two problems in carbondata: Problem 1: While encountering problems, we should close all the streams instead of only close current one. Problem 2: Current FileReader is not singleton nor the fileCache variable is a class variable, so it can only provide cache function if we pass the object among the methods. After reading version info, the fileReader cannot be used by other methods, so we must close it.
bf966bc
to
787fa98
Compare
Build Success with Spark 2.3.1, Please check CI http://136.243.101.176:8080/job/carbondataprbuilder2.3/9372/ |
Build Success with Spark 2.1.0, Please check CI http://136.243.101.176:8080/job/ApacheCarbonPRBuilder2.1/1111/ |
Build Success with Spark 2.2.1, Please check CI http://95.216.28.178:8080/job/ApacheCarbonPRBuilder1/1322/ |
Build Success with Spark 2.3.1, Please check CI http://136.243.101.176:8080/job/carbondataprbuilder2.3/9376/ |
@@ -169,7 +170,7 @@ public CarbonReaderBuilder withHadoopConf(String key, String value) { | |||
reader.initialize(split, attempt); | |||
readers.add(reader); | |||
} catch (Exception e) { | |||
reader.close(); | |||
CarbonUtil.closeStreams(readers.toArray(new RecordReader[0])); |
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 not loop and close each one in the readers
?
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.
CarbonUtil.closeStreams
will loop and close the readers. Calling this will save the loc (line of code)
LGTM |
Failed to run CarbonReaderTests in windows env. This is due to two problems in carbondata: Problem 1: While encountering problems, we should close all the streams instead of only close current one. Problem 2: Current FileReader is not singleton nor the fileCache variable is a class variable, so it can only provide cache function if we pass the object among the methods. After reading version info, the fileReader cannot be used by other methods, so we must close it. This closes #2871
Failed to run CarbonReaderTests in windows env. This is due to two
problems in carbondata:
Problem 1: While encountering problems, we should close all the streams instead of
only close current one.
Problem 2: Current FileReader is not singleton nor the fileCache
variable is a class variable, so it can only provide cache function if
we pass the object among the methods. After reading version info, the
fileReader cannot be used by other methods, so we must close it.
Be sure to do all of the following checklist to help us incorporate
your contribution quickly and easily:
NA
NA
NA
Please provide details on
- Whether new unit test cases have been added or why no new tests are required?
NA, only fix bugs
- How it is tested? Please attach test report.
Tested in local machine
- Is it a performance related change? Please attach the performance test report.
NA
- Any additional information to help reviewers in testing this change.
NA
NA