[Backport] Fix resource leaks (ComplexColumn and GenericColumn) (#3629)

[leventov]

Backport of #3629

• Remove unused ComplexColumnImpl class

• Remove throws IOException from close() in GenericColumn, ComplexColumn, IndexedFloats and IndexedLongs

• Use concise try-with-resources syntax in several places

• Fix resource leaks (ComplexColumn and GenericColumn) in SegmentAnalyzer, SearchQueryRunner, QueryableIndexIndexableAdapter and QueryableIndexStorageAdapter

• Use Closer in Iterable, returned from QueryableIndexIndexableAdapter.getRows(), in order to try to close everything even if closing some parts thew exceptions

# Conflicts:
#	processing/src/main/java/io/druid/segment/QueryableIndexIndexableAdapter.java
#	processing/src/main/java/io/druid/segment/QueryableIndexStorageAdapter.java
#	processing/src/main/java/io/druid/segment/column/SimpleColumn.java
#	processing/src/main/java/io/druid/segment/data/BlockLayoutIndexedLongSupplier.java

[nishantmonu51]

👍

[cheddar]

The name is "fix resource leaks", but the implementation appears to be just moving things to use try-with-resources or Closer instead of closeQuietly. Can you provide some more color on why closeQuietly was resulting in resource leaks?

Closer was introduced in Guava 14, but hadoop is still using 11, so using it has the potential to cause some compatibility issues.

[leventov]

@cheddar in the original PR "moving things around" is separated from actual resource leak fixes to make them move visible: #3629 fixes themselves: 1375e65

I think Closer is used extensively in the Druid codebase

[fjy]

@leventov Closer is used extensively, but purposefully avoided for any file that Hadoop indexing may touch. Agree with cheddar this needs to be tested for hadoop

[leventov]

@fjy @cheddar thanks, didn't know that. How this testing could be done?

[gianm]

Do we really need to backport this? It doesn't seem like a critical fix, and there is some risk identified. Can we leave it for 0.9.3? (which #3629 is already marked for)

[gianm]

Just asking because we are pretty late in the rc train and would like to stick to critical issues and regressions.

[leventov]

@gianm we have seen 700 000 dangling direct byte buffers on historical nodes, presumably because of these leaks. It leads to GC issues and OOMs, i. e. instability. At Metamarkets are going to use it with 0.9.2 anyway. That is why we are pretty much indifferent is it backported or not, as long as it is already in master (0.9.3 onwards).

[sascha-coenen]

we are having issues with garbage collection pauses and occasional OOMs in Druid 0.9.1.1 and would love it if this fix were to make it into the 0.9.2 release

[gianm]

Okay, based on recent comments from @leventov and @sascha-coenen the balance may shift to including this in 0.9.2. Especially since we already decided to upgrade Guice for 0.9.2 in #3222, and we already expect some people to need to adjust hadoop configs there.

[gianm]

I'm soft 👍 on including in 0.9.2-rc2. If nobody disagrees then IMO we should do it and just be on the lookout for people reporting hadoop issues with that version. We can also run it through the hadoop testing system we have been building here.

[gianm]

I ran our hadoop-gauntlet tests on this branch and they all passed:

• emr472 Passed
• emr500 Passed
• quickstart.cloudera Passed
• sandbox.hortonworks.com Passed
• sequenceiq-230 Passed
• sequenceiq-240 Passed
• sequenceiq-241 Passed
• sequenceiq-250 Passed
• sequenceiq-251 Passed
• sequenceiq-252 Passed
• sequenceiq-260 Passed
• sequenceiq-271 Passed

So I'm 👍 on backport.

[leventov]

Thanks @gianm!

[sascha-coenen]

Great! thanks a lot @gianm!