lucene-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From Shai Erera <ser...@gmail.com>
Subject Re: svn commit: r1040145 - /lucene/dev/branches/branch_3x/lucene/src/java/org/apache/lucene/store/RAMDirectory.java
Date Tue, 30 Nov 2010 04:48:32 GMT
Uwe, I'm sorry people rushed in to believe I broke backwards ... because I
don't think I did. My only mistake was that I didn't run test-backwards,
'cause I didn't really think anything can break by using CHM and not HM.

fileMap was package-private until two days ago (LUCENE-2778) when I made it
protected - therefore it wasn't a public API, right? If it wasn't public,
we're allowed to change it right? The break was only in MockRAMDir, and even
that is because I changed fileMap type from HashMap to Map, which IMO should
have been defined like that from the beginning.

Perhaps we should discuss a more general problem we have w/ the backwards
test (maybe in a dedicated thread) -- classes that access package-private
API can break if that API changes - bw-policy-wise we're allowed to make
such changes, but we cannot fix the backwards layer to compile against it
properly. Your hack is really a hack, and shouldn't be there IMO. Instead,
fixing backwards is as simple as re-compiling it. Something we could have
done when backwards had its own 'src/java' folder which we could change for
situations like that.

So the question now is - does this still count as a backwards break? I don't
think so. I don't mind if we commit your hack to MockRAMDir just to avoid
Hudson from failing. But we need to think how we can make backwards more
resilient to changes that are allowed (such as changing package-private
API).

Shai

On Tue, Nov 30, 2010 at 12:07 AM, Uwe Schindler <uwe@thetaphi.de> wrote:

> Attached patch fixes the backwards tests...
>
> ********* BUT ***********
>
> The change in RAMDirectory of a *protected* field brakes backwards!!!! We
> are no longer binary compatible, so I don’t think the change is good in 3.x.
> We should do this only in trunk, so please revert the 3.x change for now or
> have a sophisticated backwards layer!
>
> ********* BUT ***********
>
> Uwe
>
> -----
> Uwe Schindler
> H.-H.-Meier-Allee 63, D-28213 Bremen
> http://www.thetaphi.de
> eMail: uwe@thetaphi.de
>
> > From: Uwe Schindler [mailto:uwe@thetaphi.de]
> > Sent: Monday, November 29, 2010 10:05 PM
> > To: dev@lucene.apache.org
> > Subject: RE: svn commit: r1040145 -
> > /lucene/dev/branches/branch_3x/lucene/src/java/org/apache/lucene/store
> > /RAMDirectory.java
> >
> > This commit broke backwards in MockRAMDir. A fix would be to change
> > MockRAMDir in backwards to use reflection to get the Map (like used at
> > another place in MockRAMDir already).
> >
> > Uwe
> >
> > -----
> > Uwe Schindler
> > H.-H.-Meier-Allee 63, D-28213 Bremen
> > http://www.thetaphi.de
> > eMail: uwe@thetaphi.de
> >
> >
> > > -----Original Message-----
> > > From: shaie@apache.org [mailto:shaie@apache.org]
> > > Sent: Monday, November 29, 2010 4:19 PM
> > > To: commits@lucene.apache.org
> > > Subject: svn commit: r1040145 -
> > >
> > /lucene/dev/branches/branch_3x/lucene/src/java/org/apache/lucene/store
> > > /RAMDirectory.java
> > >
> > > Author: shaie
> > > Date: Mon Nov 29 15:18:42 2010
> > > New Revision: 1040145
> > >
> > > URL: http://svn.apache.org/viewvc?rev=1040145&view=rev
> > > Log:
> > > LUCENE-2779: Use ConcurrentHashMap in RAMDirectory (3x)
> > >
> > > Modified:
> > >
> > >
> > lucene/dev/branches/branch_3x/lucene/src/java/org/apache/lucene/store/
> > > RAMDirectory.java
> > >
> > > Modified:
> > >
> > lucene/dev/branches/branch_3x/lucene/src/java/org/apache/lucene/store/
> > > RAMDirectory.java
> > > URL:
> > >
> > http://svn.apache.org/viewvc/lucene/dev/branches/branch_3x/lucene/src/
> > >
> > java/org/apache/lucene/store/RAMDirectory.java?rev=1040145&r1=1040144
> > > &r2=1040145&view=diff
> > >
> > ==========================================================
> > > ====================
> > > ---
> > >
> > lucene/dev/branches/branch_3x/lucene/src/java/org/apache/lucene/store/
> > > RAMDirectory.java (original)
> > > +++
> > >
> > lucene/dev/branches/branch_3x/lucene/src/java/org/apache/lucene/store/
> > > RAMDirectory.java Mon Nov 29 15:18:42 2010 @@ -20,8 +20,8 @@ package
> > > org.apache.lucene.store;  import java.io.IOException;  import
> > > java.io.FileNotFoundException;  import java.io.Serializable; -import
> > > java.util.HashMap; -import java.util.Set;
> > > +import java.util.Map;
> > > +import java.util.concurrent.ConcurrentHashMap;
> > >  import java.util.concurrent.atomic.AtomicLong;
> > >
> > >  import org.apache.lucene.index.IndexFileNameFilter;
> > > @@ -36,7 +36,7 @@ public class RAMDirectory extends Direct
> > >
> > >    private static final long serialVersionUID = 1l;
> > >
> > > -  protected HashMap<String,RAMFile> fileMap = new
> > > HashMap<String,RAMFile>();
> > > +  protected Map<String,RAMFile> fileMap = new
> > > ConcurrentHashMap<String,RAMFile>();
> > >    protected final AtomicLong sizeInBytes = new AtomicLong();
> > >
> > >    // *****
> > > @@ -83,25 +83,16 @@ public class RAMDirectory extends Direct
> > >    }
> > >
> > >    @Override
> > > -  public synchronized final String[] listAll() {
> > > +  public final String[] listAll() {
> > >      ensureOpen();
> > > -    Set<String> fileNames = fileMap.keySet();
> > > -    String[] result = new String[fileNames.size()];
> > > -    int i = 0;
> > > -    for(final String fileName: fileNames)
> > > -      result[i++] = fileName;
> > > -    return result;
> > > +    return fileMap.keySet().toArray(new String[0]);
> > >    }
> > >
> > >    /** Returns true iff the named file exists in this directory. */
> > >    @Override
> > >    public final boolean fileExists(String name) {
> > >      ensureOpen();
> > > -    RAMFile file;
> > > -    synchronized (this) {
> > > -      file = fileMap.get(name);
> > > -    }
> > > -    return file != null;
> > > +    return fileMap.containsKey(name);
> > >    }
> > >
> > >    /** Returns the time the named file was last modified.
> > > @@ -110,12 +101,10 @@ public class RAMDirectory extends Direct
> > >    @Override
> > >    public final long fileModified(String name) throws IOException {
> > >      ensureOpen();
> > > -    RAMFile file;
> > > -    synchronized (this) {
> > > -      file = fileMap.get(name);
> > > -    }
> > > -    if (file==null)
> > > +    RAMFile file = fileMap.get(name);
> > > +    if (file == null) {
> > >        throw new FileNotFoundException(name);
> > > +    }
> > >      return file.getLastModified();
> > >    }
> > >
> > > @@ -125,12 +114,10 @@ public class RAMDirectory extends Direct
> > >    @Override
> > >    public void touchFile(String name) throws IOException {
> > >      ensureOpen();
> > > -    RAMFile file;
> > > -    synchronized (this) {
> > > -      file = fileMap.get(name);
> > > -    }
> > > -    if (file==null)
> > > +    RAMFile file = fileMap.get(name);
> > > +    if (file == null) {
> > >        throw new FileNotFoundException(name);
> > > +    }
> > >
> > >      long ts2, ts1 = System.currentTimeMillis();
> > >      do {
> > > @@ -151,19 +138,18 @@ public class RAMDirectory extends Direct
> > >    @Override
> > >    public final long fileLength(String name) throws IOException {
> > >      ensureOpen();
> > > -    RAMFile file;
> > > -    synchronized (this) {
> > > -      file = fileMap.get(name);
> > > -    }
> > > -    if (file==null)
> > > +    RAMFile file = fileMap.get(name);
> > > +    if (file == null) {
> > >        throw new FileNotFoundException(name);
> > > +    }
> > >      return file.getLength();
> > >    }
> > >
> > > -  /** Return total size in bytes of all files in this
> > > -   * directory.  This is currently quantized to
> > > -   * RAMOutputStream.BUFFER_SIZE. */
> > > -  public synchronized final long sizeInBytes() {
> > > +  /**
> > > +   * Return total size in bytes of all files in this directory. This
> is
> > > +   * currently quantized to RAMOutputStream.BUFFER_SIZE.
> > > +   */
> > > +  public final long sizeInBytes() {
> > >      ensureOpen();
> > >      return sizeInBytes.get();
> > >    }
> > > @@ -172,14 +158,15 @@ public class RAMDirectory extends Direct
> > >     * @throws IOException if the file does not exist
> > >     */
> > >    @Override
> > > -  public synchronized void deleteFile(String name) throws IOException
> > > {
> > > +  public void deleteFile(String name) throws IOException {
> > >      ensureOpen();
> > >      RAMFile file = fileMap.remove(name);
> > > -    if (file!=null) {
> > > -        file.directory = null;
> > > -        sizeInBytes.addAndGet(-file.sizeInBytes);
> > > -    } else
> > > +    if (file != null) {
> > > +      file.directory = null;
> > > +      sizeInBytes.addAndGet(-file.sizeInBytes);
> > > +    } else {
> > >        throw new FileNotFoundException(name);
> > > +    }
> > >    }
> > >
> > >    /** Creates a new, empty file in the directory with the given name.
> > > Returns a stream writing this file. */ @@ -187,14 +174,12 @@ public
> > > class RAMDirectory extends Direct
> > >    public IndexOutput createOutput(String name) throws IOException {
> > >      ensureOpen();
> > >      RAMFile file = newRAMFile();
> > > -    synchronized (this) {
> > > -      RAMFile existing = fileMap.get(name);
> > > -      if (existing!=null) {
> > > -        sizeInBytes.addAndGet(-existing.sizeInBytes);
> > > -        existing.directory = null;
> > > -      }
> > > -      fileMap.put(name, file);
> > > +    RAMFile existing = fileMap.remove(name);
> > > +    if (existing != null) {
> > > +      sizeInBytes.addAndGet(-existing.sizeInBytes);
> > > +      existing.directory = null;
> > >      }
> > > +    fileMap.put(name, file);
> > >      return new RAMOutputStream(file);
> > >    }
> > >
> > > @@ -211,12 +196,10 @@ public class RAMDirectory extends Direct
> > >    @Override
> > >    public IndexInput openInput(String name) throws IOException {
> > >      ensureOpen();
> > > -    RAMFile file;
> > > -    synchronized (this) {
> > > -      file = fileMap.get(name);
> > > -    }
> > > -    if (file == null)
> > > +    RAMFile file = fileMap.get(name);
> > > +    if (file == null) {
> > >        throw new FileNotFoundException(name);
> > > +    }
> > >      return new RAMInputStream(file);
> > >    }
> > >
> > >
> >
> >
> >
> > ---------------------------------------------------------------------
> > To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org For additional
> > commands, e-mail: dev-help@lucene.apache.org
>
>
>
> ---------------------------------------------------------------------
> To unsubscribe, e-mail: dev-unsubscribe@lucene.apache.org
> For additional commands, e-mail: dev-help@lucene.apache.org
>

Mime
View raw message