vrozov commented on a change in pull request #1333: DRILL-6410: Memory leak in
Parquet Reader during cancellation
URL: https://github.com/apache/drill/pull/1333#discussion_r203570718
##########
File path:
exec/java-exec/src/main/java/org/apache/drill/exec/store/parquet/columnreaders/AsyncPageReader.java
##########
@@ -83,21 +80,18 @@
class AsyncPageReader extends PageReader {
static final org.slf4j.Logger logger =
org.slf4j.LoggerFactory.getLogger(AsyncPageReader.class);
- private ExecutorService threadPool;
- private long queueSize;
- private LinkedBlockingQueue<ReadStatus> pageQueue;
- private ConcurrentLinkedQueue<Future<Void>> asyncPageRead;
- private long totalPageValuesRead = 0;
- private Object pageQueueSyncronize = new Object(); // Object to use to
synchronize access to the page Queue.
- // FindBugs complains if
we synchronize on a Concurrent Queue
+ private final ExecutableTasksLatch<AsyncPageReaderTask> executableTasksLatch;
Review comment:
- IMO, most of the time, the source code should document itself and java
documentation is necessary for an API distributed as an already compiled
library/jar.
- I would prefer reviewers to point to obscure code rather than to rely on
the documentation.
- Waiting for a review comments helps to avoid inconsistency between the
code and the documentation (quite a common problem) as usually code evolves
faster and documentation lags behind.
- I already added documentation for `ExecutableTasksLatch` and
`ExecutableTask`.
- I'll change comments for `AsyncPageReader` during rebase and merge
conflict resolution.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
[email protected]
With regards,
Apache Git Services
