Github user markap14 commented on a diff in the pull request:
https://github.com/apache/nifi/pull/1202#discussion_r88706738
--- Diff:
nifi-nar-bundles/nifi-provenance-repository-bundle/nifi-persistent-provenance-repository/src/main/java/org/apache/nifi/provenance/AbstractRecordWriter.java
---
@@ -0,0 +1,173 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one or more
+ * contributor license agreements. See the NOTICE file distributed with
+ * this work for additional information regarding copyright ownership.
+ * The ASF licenses this file to You under the Apache License, Version 2.0
+ * (the "License"); you may not use this file except in compliance with
+ * the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.apache.nifi.provenance;
+
+import java.io.File;
+import java.io.IOException;
+import java.io.OutputStream;
+import java.util.concurrent.locks.Lock;
+import java.util.concurrent.locks.ReentrantLock;
+
+import org.apache.nifi.provenance.serialization.RecordWriter;
+import org.apache.nifi.provenance.toc.TocWriter;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public abstract class AbstractRecordWriter implements RecordWriter {
+ private static final Logger logger =
LoggerFactory.getLogger(AbstractRecordWriter.class);
+
+ private final File file;
+ private final TocWriter tocWriter;
+ private final Lock lock = new ReentrantLock();
+
+ private volatile boolean dirty = false;
+ private volatile boolean closed = false;
+
+ private int recordsWritten = 0;
+
+ public AbstractRecordWriter(final File file, final TocWriter writer)
throws IOException {
+ logger.trace("Creating Record Writer for {}", file);
+
+ this.file = file;
+ this.tocWriter = writer;
+ }
+
+ @Override
+ public synchronized void close() throws IOException {
+ closed = true;
+
+ logger.trace("Closing Record Writer for {}", file == null ? null :
file.getName());
+
+ lock();
--- End diff --
The 'synchronized' and the lock are there to protected two different
things. The writer exposes an ability to lock it externally so that multiple
operations (such as writeRecord, flush, etc) can be called atomically without
anything else being written. The synchronized protects a few different member
variables. Essentially, it's employing two completely disparate synchronization
barriers in order to improve the throughput (no need to wait for a writer to
finish writing many records and flush before returning the number of records
written via getRecordsWritten() ).
I believe the code was more clear before I separated the writers out into
abstract classes. As they are now, it is a bit confusing and perhaps is worth
simply using the lock and not synchronizing for simplicity purposes. However, I
would be very hesitant to refactor something like that now, as this ticket is
blocking the 1.1.0 release, and I believe it is correct as-is. It is simply an
abstraction of existing classes to produce more layers to avoid code repetition.
---
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 [email protected] or file a JIRA ticket
with INFRA.
---
