Github user markap14 commented on a diff in the pull request:

    https://github.com/apache/nifi/pull/1202#discussion_r88325829
  
    --- Diff: 
nifi-commons/nifi-write-ahead-log/src/main/java/org/wali/MinimalLockingWriteAheadLog.java
 ---
    @@ -512,25 +525,38 @@ public synchronized int checkpoint() throws 
IOException {
                     swapLocations = new HashSet<>(externalLocations);
                     for (final Partition<T> partition : partitions) {
                         try {
    -                        partition.rollover();
    +                        partitionStreams.add(partition.rollover());
                         } catch (final Throwable t) {
                             partition.blackList();
                             numberBlackListedPartitions.getAndIncrement();
                             throw t;
                         }
                     }
    -
    -                // notify global sync with the write lock held. We do this 
because we don't want the repository to get updated
    -                // while the listener is performing its necessary tasks
    -                if (syncListener != null) {
    -                    syncListener.onGlobalSync();
    -                }
                 } finally {
                     writeLock.unlock();
                 }
     
                 stopTheWorldNanos = System.nanoTime() - stopTheWorldStart;
     
    +            // Close all of the Partitions' Output Streams. We do this 
here, instead of in Partition.rollover()
    +            // because we want to do this outside of the write lock. 
Because calling close() on FileOutputStream can
    +            // be very expensive, as it has to flush the data to disk, we 
don't want to prevent other Process Sessions
    +            // from getting committed. Since rollover() transitions the 
partition to write to a new file already, there
    +            // is no reason that we need to close this FileOutputStream 
before releasing the write lock. Also, if any Exception
    +            // does get thrown when calling close(), we don't need to 
blacklist the partition, as the stream that was getting
    +            // closed is not the stream being written to for the partition 
anyway.
    +            for (final OutputStream partitionStream : partitionStreams) {
    +                partitionStream.close();
    --- End diff --
    
    If close() fails we do want to throw an Exception. But you're right - we 
should catch the Exception first and close the other streams so that there is 
no resource leak. Will update that.


---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---

Reply via email to