You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@jena.apache.org by afs <gi...@git.apache.org> on 2018/08/10 20:05:07 UTC

[GitHub] jena pull request #459: Fuseki code maintenance.

GitHub user afs opened a pull request:

    https://github.com/apache/jena/pull/459

    Fuseki code maintenance.

    This follows on from PR #458 so it shows the commits for that PR in the comparison to Jena master.
    
    It is the 3 individual commits after "Delete databases and expel from JVM".
    
    It is a number of internal tidy-ups and updates of internal operations following from the new Jena transaction TxnType. For uploading, there are comments about file upload and moving the multitarget upload code to the `Upload` library.

You can merge this pull request into a Git repository by running:

    $ git pull https://github.com/afs/jena fuseki-code-maint

Alternatively you can review and apply these changes as the patch at:

    https://github.com/apache/jena/pull/459.patch

To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:

    This closes #459
    
----
commit 5a51311bd5b4ab2daf2f9cb9bb8a42a9d372e325
Author: Andy Seaborne <an...@...>
Date:   2018-08-10T15:21:39Z

    JENA-1481, JENA-1586: Delete databases and expel from JVM

commit ac9c769ae2ccaa8826a2a0e7e83bb9f0b3ebeb72
Author: Andy Seaborne <an...@...>
Date:   2018-08-10T19:43:01Z

    Update transactional usage to full range of modes.

commit 4df3d2b1a0ee3ecf4ad87af29355a8504241d658
Author: Andy Seaborne <an...@...>
Date:   2018-08-10T19:43:32Z

    Tidy

commit 8a29cefc7170a176cb59e549bf178802ac7c3db9
Author: Andy Seaborne <an...@...>
Date:   2018-08-10T19:44:46Z

    Migrate the general file uploader to library Upload.
    
    A small step towards merging the code and enabling more
    direct transactional processing.

----


---

[GitHub] jena pull request #459: Fuseki code maintenance.

Posted by kinow <gi...@git.apache.org>.
Github user kinow commented on a diff in the pull request:

    https://github.com/apache/jena/pull/459#discussion_r209416586
  
    --- Diff: jena-db/jena-tdb2/src/main/java/org/apache/jena/tdb2/sys/TDBInternal.java ---
    @@ -176,6 +176,23 @@ public static synchronized void expel(DatasetGraph dsg) {
             StoreConnection.internalExpel(locStorage, false);
         }
     
    +    /** Stop managing a DatasetGraph. Use with great care. */
    +    public static synchronized void expel(DatasetGraph dsg, boolean force) {
    +        Location locContainer = null;
    +        Location locStorage = null;
    +        
    +        if ( dsg instanceof DatasetGraphSwitchable ) {
    +            locContainer = ((DatasetGraphSwitchable)dsg).getLocation();
    +            dsg = ((DatasetGraphSwitchable)dsg).getWrapped();
    +        }
    +        if ( dsg instanceof DatasetGraphTDB )
    +            locStorage = ((DatasetGraphTDB)dsg).getLocation();
    +        
    --- End diff --
    
    Should we add something like 
    
    ```java
    if (locContainer) throw new SomeRuntimeException("Container Location cannot be null"); // NPE when we try to get from cache if null, in `DatabaseConnection.internalExpel` ?
    ```
    
    And ditto for the `locStorage` ? Otherwise the `ConcurrentHashMap`'s `cache` used will throw a NPE (I think...)


---

[GitHub] jena pull request #459: Fuseki code maintenance.

Posted by afs <gi...@git.apache.org>.
Github user afs commented on a diff in the pull request:

    https://github.com/apache/jena/pull/459#discussion_r209422526
  
    --- Diff: jena-db/jena-tdb2/src/main/java/org/apache/jena/tdb2/sys/TDBInternal.java ---
    @@ -176,6 +176,23 @@ public static synchronized void expel(DatasetGraph dsg) {
             StoreConnection.internalExpel(locStorage, false);
         }
     
    +    /** Stop managing a DatasetGraph. Use with great care. */
    +    public static synchronized void expel(DatasetGraph dsg, boolean force) {
    +        Location locContainer = null;
    +        Location locStorage = null;
    +        
    +        if ( dsg instanceof DatasetGraphSwitchable ) {
    +            locContainer = ((DatasetGraphSwitchable)dsg).getLocation();
    +            dsg = ((DatasetGraphSwitchable)dsg).getWrapped();
    +        }
    +        if ( dsg instanceof DatasetGraphTDB )
    +            locStorage = ((DatasetGraphTDB)dsg).getLocation();
    +        
    --- End diff --
    
    Good point.
    
    Added `if ( locContainer != null )` -- it might be null if passing in a `DatasetGraphTDB` (an unusual, internal case).
    
    `locStorage` is not null.  Even in-memory have a `Location`.
    
    As it says "Use with great care"!


---

[GitHub] jena pull request #459: Fuseki code maintenance.

Posted by kinow <gi...@git.apache.org>.
Github user kinow commented on a diff in the pull request:

    https://github.com/apache/jena/pull/459#discussion_r209416335
  
    --- Diff: jena-fuseki2/jena-fuseki-core/src/main/java/org/apache/jena/fuseki/servlets/SPARQL_Upload.java ---
    @@ -20,35 +20,39 @@
     
     import static java.lang.String.format ;
     
    -import java.io.InputStream ;
     import java.io.PrintWriter ;
    -import java.util.zip.GZIPInputStream ;
     
     import javax.servlet.http.HttpServletRequest ;
     import javax.servlet.http.HttpServletResponse ;
     
    -import org.apache.commons.fileupload.FileItemIterator ;
    -import org.apache.commons.fileupload.FileItemStream ;
     import org.apache.commons.fileupload.servlet.ServletFileUpload ;
    -import org.apache.commons.fileupload.util.Streams ;
    -import org.apache.jena.atlas.web.ContentType ;
     import org.apache.jena.fuseki.Fuseki ;
     import org.apache.jena.fuseki.FusekiLib ;
    +import org.apache.jena.fuseki.system.Upload;
    +import org.apache.jena.fuseki.system.UploadDetailsWithName;
     import org.apache.jena.graph.Node ;
     import org.apache.jena.graph.NodeFactory ;
    -import org.apache.jena.iri.IRI ;
    -import org.apache.jena.riot.Lang ;
    -import org.apache.jena.riot.RDFLanguages ;
    -import org.apache.jena.riot.lang.StreamRDFCounting ;
    -import org.apache.jena.riot.system.IRIResolver ;
    -import org.apache.jena.riot.system.StreamRDF ;
    -import org.apache.jena.riot.system.StreamRDFLib ;
     import org.apache.jena.riot.web.HttpNames ;
     import org.apache.jena.sparql.core.DatasetGraph ;
    -import org.apache.jena.sparql.core.DatasetGraphFactory ;
     import org.apache.jena.sparql.core.Quad ;
     import org.apache.jena.web.HttpSC ;
     
    +/**
    + * Upload data into a graph within a dataset. This is fuseki:serviceUpload.
    + * 
    + * It is better to use GSP POST with the body being the content.
    + * 
    + * This class work with general HTML form file upload wherte  has the name somewhere in the form and that may be
    --- End diff --
    
    s/class work/class works/
    
    s/wherte  has/where it has/


---

[GitHub] jena pull request #459: Fuseki code maintenance.

Posted by afs <gi...@git.apache.org>.
Github user afs commented on a diff in the pull request:

    https://github.com/apache/jena/pull/459#discussion_r209421902
  
    --- Diff: jena-fuseki2/jena-fuseki-core/src/main/java/org/apache/jena/fuseki/servlets/SPARQL_Upload.java ---
    @@ -20,35 +20,39 @@
     
     import static java.lang.String.format ;
     
    -import java.io.InputStream ;
     import java.io.PrintWriter ;
    -import java.util.zip.GZIPInputStream ;
     
     import javax.servlet.http.HttpServletRequest ;
     import javax.servlet.http.HttpServletResponse ;
     
    -import org.apache.commons.fileupload.FileItemIterator ;
    -import org.apache.commons.fileupload.FileItemStream ;
     import org.apache.commons.fileupload.servlet.ServletFileUpload ;
    -import org.apache.commons.fileupload.util.Streams ;
    -import org.apache.jena.atlas.web.ContentType ;
     import org.apache.jena.fuseki.Fuseki ;
     import org.apache.jena.fuseki.FusekiLib ;
    +import org.apache.jena.fuseki.system.Upload;
    +import org.apache.jena.fuseki.system.UploadDetailsWithName;
     import org.apache.jena.graph.Node ;
     import org.apache.jena.graph.NodeFactory ;
    -import org.apache.jena.iri.IRI ;
    -import org.apache.jena.riot.Lang ;
    -import org.apache.jena.riot.RDFLanguages ;
    -import org.apache.jena.riot.lang.StreamRDFCounting ;
    -import org.apache.jena.riot.system.IRIResolver ;
    -import org.apache.jena.riot.system.StreamRDF ;
    -import org.apache.jena.riot.system.StreamRDFLib ;
     import org.apache.jena.riot.web.HttpNames ;
     import org.apache.jena.sparql.core.DatasetGraph ;
    -import org.apache.jena.sparql.core.DatasetGraphFactory ;
     import org.apache.jena.sparql.core.Quad ;
     import org.apache.jena.web.HttpSC ;
     
    +/**
    + * Upload data into a graph within a dataset. This is fuseki:serviceUpload.
    + * 
    + * It is better to use GSP POST with the body being the content.
    + * 
    + * This class work with general HTML form file upload wherte  has the name somewhere in the form and that may be
    + * after the data.
    + * 
    + * With sophisticated use of {@link ServletFileUpload}, it is possible to stream to disk
    + * avoid an in-memory copy. The whole form is processed to find the fields before parsing starts
    + * and potentially is is several files.
    --- End diff --
    
    Fixed. 
    
    Also - moved much of the comment text into Upload. With all the code moving the more implement-centric comment should move as well.
    
    (Sorting out `Upload` itself is another task for some other time. Code reorg is small step towards that.)



---

[GitHub] jena pull request #459: Fuseki code maintenance.

Posted by kinow <gi...@git.apache.org>.
Github user kinow commented on a diff in the pull request:

    https://github.com/apache/jena/pull/459#discussion_r209416490
  
    --- Diff: jena-fuseki2/jena-fuseki-core/src/main/java/org/apache/jena/fuseki/server/DataService.java ---
    @@ -151,50 +151,24 @@ public long getRequestsBad() {
             return counters.value(CounterName.RequestsBad) ;
         }
     
    -    /** Counter of active read transactions */
    -    public AtomicLong   activeReadTxn           = new AtomicLong(0) ;
    +    /** Counter of active transactions */
    +    public AtomicLong   activeTxn           = new AtomicLong(0) ;
     
    -    /** Counter of active write transactions */
    -    public AtomicLong   activeWriteTxn          = new AtomicLong(0) ;
    +    /** Cumulative counter of transactions */
    +    public AtomicLong   totalTxn            = new AtomicLong(0) ;
     
    -    /** Cumulative counter of read transactions */
    -    public AtomicLong   totalReadTxn            = new AtomicLong(0) ;
    -
    -    /** Cumulative counter of writer transactions */
    -    public AtomicLong   totalWriteTxn           = new AtomicLong(0) ;
    -
    -    public void startTxn(ReadWrite mode)
    -    {
    -        switch(mode)
    -        {
    -            case READ:  
    -                activeReadTxn.getAndIncrement() ;
    -                totalReadTxn.getAndIncrement() ;
    -                break ;
    -            case WRITE:
    -                activeWriteTxn.getAndIncrement() ;
    -                totalWriteTxn.getAndIncrement() ;
    -                break ;
    -        }
    +    public void startTxn(TxnType mode) {
    +        activeTxn.getAndIncrement();
    +        totalTxn.getAndIncrement();
         }
     
    -    public void finishTxn(ReadWrite mode)
    --- End diff --
    
    Should we also remove unused `#checkShutdown` and `requestCounter` ?


---

[GitHub] jena pull request #459: Fuseki code maintenance.

Posted by afs <gi...@git.apache.org>.
Github user afs commented on a diff in the pull request:

    https://github.com/apache/jena/pull/459#discussion_r209422540
  
    --- Diff: jena-base/src/main/java/org/apache/jena/atlas/io/IO.java ---
    @@ -364,4 +369,31 @@ public static String uniqueFilename(String directory, String base, String ext) {
                 return null ;
             }
         }
    +    
    +    /** Delete everything from a {@code Path} start point, including the path itself.
    +     * This function works on files or directories.
    +     * This function does not follow symbolic links.
    +     */  
    +    public static void deleteAll(Path start) {
    --- End diff --
    
    A symbolic link should be deleted, that is, the entry in the parent directory is removed, and not the thing it points to.  c.f. `rm`.  The idea being if you put in a symbolic link, the target may be pointed to from elsewhere as well.
    
    I had looked at FileUtils as well, I think it deletes the link and not the target (it calls `File.delete`).



---

[GitHub] jena pull request #459: Fuseki code maintenance.

Posted by kinow <gi...@git.apache.org>.
Github user kinow commented on a diff in the pull request:

    https://github.com/apache/jena/pull/459#discussion_r209416401
  
    --- Diff: jena-fuseki2/jena-fuseki-core/src/main/java/org/apache/jena/fuseki/servlets/HttpAction.java ---
    @@ -18,8 +18,8 @@
     
     package org.apache.jena.fuseki.servlets;
     
    -import static org.apache.jena.query.ReadWrite.READ ;
    -import static org.apache.jena.query.ReadWrite.WRITE ;
    +import static org.apache.jena.query.TxnType.*;
    --- End diff --
    
    Another nit-pick, but as we have `oaj.query.TxnType.WRITE` already import, maybe we can replace the `*` by two entries for `READ` & `READ_PROMOTE`?


---

[GitHub] jena pull request #459: Fuseki code maintenance.

Posted by afs <gi...@git.apache.org>.
Github user afs commented on a diff in the pull request:

    https://github.com/apache/jena/pull/459#discussion_r209421904
  
    --- Diff: jena-fuseki2/jena-fuseki-core/src/main/java/org/apache/jena/fuseki/servlets/HttpAction.java ---
    @@ -270,30 +269,50 @@ public boolean isTransactional() {
             return isTransactional ;
         }
     
    -    public void beginRead() {
    -        activeMode = READ ;
    -        transactional.begin(READ) ;
    +    public void begin(TxnType txnType) {
    +        transactional.begin(txnType);
             activeDSG = dsg ;
    -        dataService.startTxn(READ) ;
    +        dataService.startTxn(txnType) ;
    +    }
    +    
    +    public void begin() {
    +        begin(READ_PROMOTE);
    +    }
    +
    +    public void beginWrite() {
    +        begin(WRITE);
    +    }
    +
    +    public void beginRead() {
    +        begin(READ);
         }
     
         public void endRead() {
    -        dataService.finishTxn(READ) ;
    -        activeMode = null ;
    +        dataService.finishTxn() ;
    +        transactional.commit();
             transactional.end() ;
             activeDSG = null ;
         }
     
    -    public void beginWrite() {
    -        transactional.begin(WRITE) ;
    -        activeMode = WRITE ;
    -        activeDSG = dsg ;
    -        dataService.startTxn(WRITE) ;
    +    public void end() {
    +        dataService.finishTxn() ;
    +        
    +        if ( transactional.isInTransaction() ) {
    +            Log.warn(this, "Transaction still active - no commit or abort seen (forced abort)") ;
    +            try {
    +                transactional.abort() ;
    +            } catch (RuntimeException ex) {
    +                Log.warn(this, "Exception in forced abort (trying to continue)", ex) ;
    +            }
    +        }
    +        transactional.end() ;
    +        activeDSG = null ;
         }
    -
    +    
    --- End diff --
    
    Removed


---

[GitHub] jena pull request #459: Fuseki code maintenance.

Posted by rvesse <gi...@git.apache.org>.
Github user rvesse commented on a diff in the pull request:

    https://github.com/apache/jena/pull/459#discussion_r209539897
  
    --- Diff: jena-fuseki2/jena-fuseki-core/src/main/java/org/apache/jena/fuseki/mgt/ActionDatasets.java ---
    @@ -288,28 +294,77 @@ protected void execDeleteItem(HttpAction action) {
             if ( ! action.getDataAccessPointRegistry().isRegistered(name) )
                 ServletOps.errorNotFound("No such dataset registered: "+name);
     
    +        // This acts as a lock. 
             systemDSG.begin(ReadWrite.WRITE) ;
             boolean committed = false ;
    +
             try {
                 // Here, go offline.
                 // Need to reference count operations when they drop to zero
                 // or a timer goes off, we delete the dataset.
     
                 DataAccessPoint ref = action.getDataAccessPointRegistry().get(name) ;
    +            
                 // Redo check inside transaction.
                 if ( ref == null )
                     ServletOps.errorNotFound("No such dataset registered: "+name);
    +            
    +            String filename = name.startsWith("/") ? name.substring(1) : name;
    +            List<String> configurationFiles = FusekiSystem.existingConfigurationFile(filename);
    +            if  ( configurationFiles.size() != 1 ) {
    +                // This should not happen.
    +                action.log.warn(format("[%d] There are %d configuration files, not one.", action.id, configurationFiles.size()));
    +                ServletOps.errorOccurred(
    +                    format(
    +                        "There are %d configuration files, not one. Delete not performed; clearup of the filesystem needed.",
    +                        action.id, configurationFiles.size()));
    +            }
    +            
    +            String cfgPathname = configurationFiles.get(0);
    +            
    +            // Delete configuration file.
    +            // Once deleted, server restart will not have the database. 
    +            FileOps.deleteSilent(cfgPathname);
     
    -            // Make it invisible to the outside.
    +            // Get before removing.
    +            DataService dataService = ref.getDataService();
    +            
    +            // Make it invisible in this running server.
                 action.getDataAccessPointRegistry().remove(name);
    -            // Delete configuration file.
    -            // Should be only one, undo damage if multiple.
    -            String filename = name.startsWith("/") ? name.substring(1) : name;
    -            FusekiSystem.existingConfigurationFile(filename).stream().forEach(FileOps::deleteSilent);
    -            // Leave the database in place. if it is in /databases/, recreating the
    -            // configuration will make the database reappear. This is intentional.
    -            // Deleting a large database by accident is a major mistake.
     
    +            // Delete the database for real only when it is in the server "run/databases"
    +            // area. Don't delete databases that reside elsewhere. We do delete the
    +            // configuration file, so the databases will not be associated with the server
    +            // anymore.
    --- End diff --
    
    Where is the code that actually enforces the above comment?  I don't see any check against the location of the database in the subsequent code


---

[GitHub] jena pull request #459: Fuseki code maintenance.

Posted by kinow <gi...@git.apache.org>.
Github user kinow commented on a diff in the pull request:

    https://github.com/apache/jena/pull/459#discussion_r209416731
  
    --- Diff: jena-base/src/main/java/org/apache/jena/atlas/io/IO.java ---
    @@ -364,4 +369,31 @@ public static String uniqueFilename(String directory, String base, String ext) {
                 return null ;
             }
         }
    +    
    +    /** Delete everything from a {@code Path} start point, including the path itself.
    +     * This function works on files or directories.
    +     * This function does not follow symbolic links.
    +     */  
    +    public static void deleteAll(Path start) {
    --- End diff --
    
    `Files.delete()` is a no-no because it would remove the symlink? I had a look at `FileUtils` from Commons IO (project dependency) but not sure if there's a method that does the same as this method (and Java7's Files/Paths is a nice API).


---

[GitHub] jena pull request #459: Fuseki code maintenance.

Posted by afs <gi...@git.apache.org>.
Github user afs commented on a diff in the pull request:

    https://github.com/apache/jena/pull/459#discussion_r209421906
  
    --- Diff: jena-fuseki2/jena-fuseki-core/src/main/java/org/apache/jena/fuseki/servlets/HttpAction.java ---
    @@ -18,8 +18,8 @@
     
     package org.apache.jena.fuseki.servlets;
     
    -import static org.apache.jena.query.ReadWrite.READ ;
    -import static org.apache.jena.query.ReadWrite.WRITE ;
    +import static org.apache.jena.query.TxnType.*;
    --- End diff --
    
    Fixed - eclipse organise imports applied!


---

[GitHub] jena pull request #459: Fuseki code maintenance.

Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:

    https://github.com/apache/jena/pull/459


---

[GitHub] jena pull request #459: Fuseki code maintenance.

Posted by kinow <gi...@git.apache.org>.
Github user kinow commented on a diff in the pull request:

    https://github.com/apache/jena/pull/459#discussion_r209424635
  
    --- Diff: jena-db/jena-tdb2/src/main/java/org/apache/jena/tdb2/sys/TDBInternal.java ---
    @@ -176,6 +176,23 @@ public static synchronized void expel(DatasetGraph dsg) {
             StoreConnection.internalExpel(locStorage, false);
         }
     
    +    /** Stop managing a DatasetGraph. Use with great care. */
    +    public static synchronized void expel(DatasetGraph dsg, boolean force) {
    +        Location locContainer = null;
    +        Location locStorage = null;
    +        
    +        if ( dsg instanceof DatasetGraphSwitchable ) {
    +            locContainer = ((DatasetGraphSwitchable)dsg).getLocation();
    +            dsg = ((DatasetGraphSwitchable)dsg).getWrapped();
    +        }
    +        if ( dsg instanceof DatasetGraphTDB )
    +            locStorage = ((DatasetGraphTDB)dsg).getLocation();
    +        
    --- End diff --
    
    Makes sense! Thanks for quickly checking. Everything looks good, LGTM :+1: 


---

[GitHub] jena pull request #459: Fuseki code maintenance.

Posted by kinow <gi...@git.apache.org>.
Github user kinow commented on a diff in the pull request:

    https://github.com/apache/jena/pull/459#discussion_r209416344
  
    --- Diff: jena-fuseki2/jena-fuseki-core/src/main/java/org/apache/jena/fuseki/servlets/HttpAction.java ---
    @@ -270,30 +269,50 @@ public boolean isTransactional() {
             return isTransactional ;
         }
     
    -    public void beginRead() {
    -        activeMode = READ ;
    -        transactional.begin(READ) ;
    +    public void begin(TxnType txnType) {
    +        transactional.begin(txnType);
             activeDSG = dsg ;
    -        dataService.startTxn(READ) ;
    +        dataService.startTxn(txnType) ;
    +    }
    +    
    +    public void begin() {
    +        begin(READ_PROMOTE);
    +    }
    +
    +    public void beginWrite() {
    +        begin(WRITE);
    +    }
    +
    +    public void beginRead() {
    +        begin(READ);
         }
     
         public void endRead() {
    -        dataService.finishTxn(READ) ;
    -        activeMode = null ;
    +        dataService.finishTxn() ;
    +        transactional.commit();
             transactional.end() ;
             activeDSG = null ;
         }
     
    -    public void beginWrite() {
    -        transactional.begin(WRITE) ;
    -        activeMode = WRITE ;
    -        activeDSG = dsg ;
    -        dataService.startTxn(WRITE) ;
    +    public void end() {
    +        dataService.finishTxn() ;
    +        
    +        if ( transactional.isInTransaction() ) {
    +            Log.warn(this, "Transaction still active - no commit or abort seen (forced abort)") ;
    +            try {
    +                transactional.abort() ;
    +            } catch (RuntimeException ex) {
    +                Log.warn(this, "Exception in forced abort (trying to continue)", ex) ;
    +            }
    +        }
    +        transactional.end() ;
    +        activeDSG = null ;
         }
    -
    +    
    --- End diff --
    
    Nit-pick, unnecessary spaces?


---

[GitHub] jena pull request #459: Fuseki code maintenance.

Posted by afs <gi...@git.apache.org>.
Github user afs commented on a diff in the pull request:

    https://github.com/apache/jena/pull/459#discussion_r209421898
  
    --- Diff: jena-fuseki2/jena-fuseki-core/src/main/java/org/apache/jena/fuseki/servlets/SPARQL_Upload.java ---
    @@ -20,35 +20,39 @@
     
     import static java.lang.String.format ;
     
    -import java.io.InputStream ;
     import java.io.PrintWriter ;
    -import java.util.zip.GZIPInputStream ;
     
     import javax.servlet.http.HttpServletRequest ;
     import javax.servlet.http.HttpServletResponse ;
     
    -import org.apache.commons.fileupload.FileItemIterator ;
    -import org.apache.commons.fileupload.FileItemStream ;
     import org.apache.commons.fileupload.servlet.ServletFileUpload ;
    -import org.apache.commons.fileupload.util.Streams ;
    -import org.apache.jena.atlas.web.ContentType ;
     import org.apache.jena.fuseki.Fuseki ;
     import org.apache.jena.fuseki.FusekiLib ;
    +import org.apache.jena.fuseki.system.Upload;
    +import org.apache.jena.fuseki.system.UploadDetailsWithName;
     import org.apache.jena.graph.Node ;
     import org.apache.jena.graph.NodeFactory ;
    -import org.apache.jena.iri.IRI ;
    -import org.apache.jena.riot.Lang ;
    -import org.apache.jena.riot.RDFLanguages ;
    -import org.apache.jena.riot.lang.StreamRDFCounting ;
    -import org.apache.jena.riot.system.IRIResolver ;
    -import org.apache.jena.riot.system.StreamRDF ;
    -import org.apache.jena.riot.system.StreamRDFLib ;
     import org.apache.jena.riot.web.HttpNames ;
     import org.apache.jena.sparql.core.DatasetGraph ;
    -import org.apache.jena.sparql.core.DatasetGraphFactory ;
     import org.apache.jena.sparql.core.Quad ;
     import org.apache.jena.web.HttpSC ;
     
    +/**
    + * Upload data into a graph within a dataset. This is fuseki:serviceUpload.
    + * 
    + * It is better to use GSP POST with the body being the content.
    + * 
    + * This class work with general HTML form file upload wherte  has the name somewhere in the form and that may be
    --- End diff --
    
    Fixed, thanks for spotting it.


---

[GitHub] jena pull request #459: Fuseki code maintenance.

Posted by kinow <gi...@git.apache.org>.
Github user kinow commented on a diff in the pull request:

    https://github.com/apache/jena/pull/459#discussion_r209416339
  
    --- Diff: jena-fuseki2/jena-fuseki-core/src/main/java/org/apache/jena/fuseki/servlets/SPARQL_Upload.java ---
    @@ -20,35 +20,39 @@
     
     import static java.lang.String.format ;
     
    -import java.io.InputStream ;
     import java.io.PrintWriter ;
    -import java.util.zip.GZIPInputStream ;
     
     import javax.servlet.http.HttpServletRequest ;
     import javax.servlet.http.HttpServletResponse ;
     
    -import org.apache.commons.fileupload.FileItemIterator ;
    -import org.apache.commons.fileupload.FileItemStream ;
     import org.apache.commons.fileupload.servlet.ServletFileUpload ;
    -import org.apache.commons.fileupload.util.Streams ;
    -import org.apache.jena.atlas.web.ContentType ;
     import org.apache.jena.fuseki.Fuseki ;
     import org.apache.jena.fuseki.FusekiLib ;
    +import org.apache.jena.fuseki.system.Upload;
    +import org.apache.jena.fuseki.system.UploadDetailsWithName;
     import org.apache.jena.graph.Node ;
     import org.apache.jena.graph.NodeFactory ;
    -import org.apache.jena.iri.IRI ;
    -import org.apache.jena.riot.Lang ;
    -import org.apache.jena.riot.RDFLanguages ;
    -import org.apache.jena.riot.lang.StreamRDFCounting ;
    -import org.apache.jena.riot.system.IRIResolver ;
    -import org.apache.jena.riot.system.StreamRDF ;
    -import org.apache.jena.riot.system.StreamRDFLib ;
     import org.apache.jena.riot.web.HttpNames ;
     import org.apache.jena.sparql.core.DatasetGraph ;
    -import org.apache.jena.sparql.core.DatasetGraphFactory ;
     import org.apache.jena.sparql.core.Quad ;
     import org.apache.jena.web.HttpSC ;
     
    +/**
    + * Upload data into a graph within a dataset. This is fuseki:serviceUpload.
    + * 
    + * It is better to use GSP POST with the body being the content.
    + * 
    + * This class work with general HTML form file upload wherte  has the name somewhere in the form and that may be
    + * after the data.
    + * 
    + * With sophisticated use of {@link ServletFileUpload}, it is possible to stream to disk
    + * avoid an in-memory copy. The whole form is processed to find the fields before parsing starts
    + * and potentially is is several files.
    --- End diff --
    
    duplicate is? Or is in?


---

[GitHub] jena pull request #459: Fuseki code maintenance.

Posted by afs <gi...@git.apache.org>.
Github user afs commented on a diff in the pull request:

    https://github.com/apache/jena/pull/459#discussion_r209421919
  
    --- Diff: jena-fuseki2/jena-fuseki-core/src/main/java/org/apache/jena/fuseki/server/DataService.java ---
    @@ -151,50 +151,24 @@ public long getRequestsBad() {
             return counters.value(CounterName.RequestsBad) ;
         }
     
    -    /** Counter of active read transactions */
    -    public AtomicLong   activeReadTxn           = new AtomicLong(0) ;
    +    /** Counter of active transactions */
    +    public AtomicLong   activeTxn           = new AtomicLong(0) ;
     
    -    /** Counter of active write transactions */
    -    public AtomicLong   activeWriteTxn          = new AtomicLong(0) ;
    +    /** Cumulative counter of transactions */
    +    public AtomicLong   totalTxn            = new AtomicLong(0) ;
     
    -    /** Cumulative counter of read transactions */
    -    public AtomicLong   totalReadTxn            = new AtomicLong(0) ;
    -
    -    /** Cumulative counter of writer transactions */
    -    public AtomicLong   totalWriteTxn           = new AtomicLong(0) ;
    -
    -    public void startTxn(ReadWrite mode)
    -    {
    -        switch(mode)
    -        {
    -            case READ:  
    -                activeReadTxn.getAndIncrement() ;
    -                totalReadTxn.getAndIncrement() ;
    -                break ;
    -            case WRITE:
    -                activeWriteTxn.getAndIncrement() ;
    -                totalWriteTxn.getAndIncrement() ;
    -                break ;
    -        }
    +    public void startTxn(TxnType mode) {
    +        activeTxn.getAndIncrement();
    +        totalTxn.getAndIncrement();
         }
     
    -    public void finishTxn(ReadWrite mode)
    --- End diff --
    
    Removed requestCounter and checkShutdown.
    
    (And renamed DatasetStatus as DataServiceStatus becaue it is a more accurate name).
    
    More generally, looking at this and clean up code in `ActionDatasets`, I think some rewriting is called for so see a commit to come for some better handling.


---

[GitHub] jena pull request #459: Fuseki code maintenance.

Posted by afs <gi...@git.apache.org>.
Github user afs commented on a diff in the pull request:

    https://github.com/apache/jena/pull/459#discussion_r209542012
  
    --- Diff: jena-fuseki2/jena-fuseki-core/src/main/java/org/apache/jena/fuseki/mgt/ActionDatasets.java ---
    @@ -288,28 +294,77 @@ protected void execDeleteItem(HttpAction action) {
             if ( ! action.getDataAccessPointRegistry().isRegistered(name) )
                 ServletOps.errorNotFound("No such dataset registered: "+name);
     
    +        // This acts as a lock. 
             systemDSG.begin(ReadWrite.WRITE) ;
             boolean committed = false ;
    +
             try {
                 // Here, go offline.
                 // Need to reference count operations when they drop to zero
                 // or a timer goes off, we delete the dataset.
     
                 DataAccessPoint ref = action.getDataAccessPointRegistry().get(name) ;
    +            
                 // Redo check inside transaction.
                 if ( ref == null )
                     ServletOps.errorNotFound("No such dataset registered: "+name);
    +            
    +            String filename = name.startsWith("/") ? name.substring(1) : name;
    +            List<String> configurationFiles = FusekiSystem.existingConfigurationFile(filename);
    +            if  ( configurationFiles.size() != 1 ) {
    +                // This should not happen.
    +                action.log.warn(format("[%d] There are %d configuration files, not one.", action.id, configurationFiles.size()));
    +                ServletOps.errorOccurred(
    +                    format(
    +                        "There are %d configuration files, not one. Delete not performed; clearup of the filesystem needed.",
    +                        action.id, configurationFiles.size()));
    +            }
    +            
    +            String cfgPathname = configurationFiles.get(0);
    +            
    +            // Delete configuration file.
    +            // Once deleted, server restart will not have the database. 
    +            FileOps.deleteSilent(cfgPathname);
     
    -            // Make it invisible to the outside.
    +            // Get before removing.
    +            DataService dataService = ref.getDataService();
    +            
    +            // Make it invisible in this running server.
                 action.getDataAccessPointRegistry().remove(name);
    -            // Delete configuration file.
    -            // Should be only one, undo damage if multiple.
    -            String filename = name.startsWith("/") ? name.substring(1) : name;
    -            FusekiSystem.existingConfigurationFile(filename).stream().forEach(FileOps::deleteSilent);
    -            // Leave the database in place. if it is in /databases/, recreating the
    -            // configuration will make the database reappear. This is intentional.
    -            // Deleting a large database by accident is a major mistake.
     
    +            // Delete the database for real only when it is in the server "run/databases"
    +            // area. Don't delete databases that reside elsewhere. We do delete the
    +            // configuration file, so the databases will not be associated with the server
    +            // anymore.
    --- End diff --
    
    The config file is deleted with (line 327):
    ```
    FileOps.deleteSilent(cfgPathname);
    ```
    and this code line 351) looks for the direct in the "run/databases" area which is `FusekiSystem.dirDatabases`:
    
    ```
                    Path pDatabase = FusekiSystem.dirDatabases.resolve(filename);
                    if ( Files.exists(pDatabase)) {
                        try {
                            if ( Files.isSymbolicLink(pDatabase)) {
    ```


---