[ 
https://issues.apache.org/jira/browse/NIFI-2854?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=15677301#comment-15677301
 ] 

ASF GitHub Bot commented on NIFI-2854:
--------------------------------------

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.


> Enable repositories to support upgrades and rollback in well defined scenarios
> ------------------------------------------------------------------------------
>
>                 Key: NIFI-2854
>                 URL: https://issues.apache.org/jira/browse/NIFI-2854
>             Project: Apache NiFi
>          Issue Type: Improvement
>          Components: Core Framework
>            Reporter: Mark Payne
>            Assignee: Mark Payne
>             Fix For: 1.1.0
>
>
> The flowfile, swapfile, provenance, and content repositories play a very 
> important roll in NiFi's ability to be safely upgraded and rolled back.  We 
> need to have well documented behaviors, designs, and version adherence so 
> that users can safely rely on these mechanisms.
> Once this is formalized and in place we should update our versioning guidance 
> to reflect this as well.
> The following would be true from NiFi 1.2.0 onward
> * No changes to how the repositories are persisted to disk can be made which 
> will break forward/backward compatibility and specifically this means that 
> things like the way each is serialized to disk cannot change.
> * If changes are made which impact forward or backward compatibility they 
> should be reserved for major releases only and should include a utility to 
> help users with pre-existing data convert from some older format to the newer 
> format.  It may not be feasible to have rollback on major releases.
> * The content repository should not be changed within a major release cycle 
> in any way that will harm forward or backward compatibility.
> * The flow file repository can change in that new fields can be added to 
> existing write ahead log record types but no fields can be removed nor can 
> any new types be added.  Once a field is considered required it must remain 
> required.  Changes may only be made across minor version changes - not 
> incremental.
> * Swap File storage should follow very similar rules to the flow file 
> repository.  Adding a schema to the swap file header may allow some variation 
> there but the variation should only be hints to optimize how they're 
> processed and not change their behavior otherwise. Changes are only permitted 
> during minor version releases.
> * Provenance repository changes are only permitted during minor version 
> releases.  These changes may include adding or removing fields from existing 
> event types.  If a field is considered required it must always be considered 
> required.  If a field is removed then it must not be a required field and 
> there must be a sensible default an older version could use if that value is 
> not found in new data once rolled back.  New event types may be added.  
> Fields or event types not known to older version, if seen after a rollback, 
> will simply be ignored.



--
This message was sent by Atlassian JIRA
(v6.3.4#6332)

Reply via email to