Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

REVIEW: NXCM-4867 - Index update concurrency problem #848

Merged
merged 9 commits into from

4 participants

@scarlucci

https://issues.sonatype.org/browse/NXCM-4867

These changes fix a synchronization problem caused by attempting an incremental index update when an incremental update is not possible, and a full index update must be performed. In this case, since the maven indexer did not support a way to request an incremental-only update, it was not possible to know the appropriate locking mechanism to use for the update.

Previously, we were grabbing a read-only lock to perform the incremental update. But in cases where an incremental was not possible, and a full index update was performed, there was a window towards the end of the full index update where another thread attempting to read the index could experience an NPE.

The maven indexer has been updated to allow clients to specify an incremental-only index update. These changes take advantage of this new support. If an incremental update is not possible, then a full index update is performed with the appropriate locking

CI build on the way

scarlucci added some commits
@scarlucci scarlucci Switch to new IndexUpdater interfaces 398ebcd
@scarlucci scarlucci Get index update working with new update interfaces 73e71f5
@scarlucci scarlucci Update maven indexer version 666f8ce
@scarlucci scarlucci Updates based on changes to index updater interface abb26d9
@scarlucci scarlucci Switch to new pseudo-release of maven indexer
Switch to new pseudo-release of maven indexer, which adds support for
running an incremental-only index update
2fc19c9
@scarlucci scarlucci Merge remote-tracking branch 'origin/master' into NXCM-4867-indexer-s…
…ync-simple

Conflicts:
	plugins/indexer/nexus-indexer-lucene-plugin/pom.xml
	plugins/indexer/nexus-indexer-lucene-plugin/src/main/java/org/sonatype/nexus/index/DefaultIndexerManager.java
d877031
@ifedorenko ifedorenko commented on the diff
...a/org/sonatype/nexus/index/DefaultIndexerManager.java
@@ -2800,4 +2858,14 @@ public IndexingContext getRepositoryIndexContext( String repositoryId )
{
return mavenIndexer.getIndexingContexts().get( getContextId( repositoryId ) );
}
+

I recommend changing exception class name to IndexUpdateException. Please also mark the class as static as it does not and should not need access any member of the outer class.

Changed the class to be static. Left the name to be as explicit as possible about the conditions under which this exception is used, but can be convinced to change if someone has a strong opinion about this name

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@ifedorenko

+1

Couple of cosmetic comments, but I don't feel too strong about them so feel free to ignore.

@kellyrob99 kellyrob99 commented on the diff
plugins/indexer/nexus-indexer-lucene-plugin/pom.xml
@@ -30,8 +30,7 @@
<properties>
<pluginName>Nexus Indexer Lucene Plugin</pluginName>
<pluginDescription>Adds search capabilities for repository content.</pluginDescription>
-
- <mavenIndexer.version>5.1.2-c65990c</mavenIndexer.version>
+ <mavenIndexer.version>5.1.2-bf3d48b</mavenIndexer.version>
@kellyrob99 Owner

Just out of curiousity, why the strange version number?

@cstamas Owner
cstamas added a note

These are "pseudo" releases of ASF Maven Indexer. ASF MI 5.1.1 was released last, codebase is currently on 5.1.2-SNAPSHOT. The "bf3d48b" suffix is basically the Git sha1 commit hash out of which the pseudo release of MI was done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@scarlucci scarlucci Change inner class to static
Inner class does not need access to outer class
9cf8d21
@kellyrob99
Owner

+1

@scarlucci scarlucci Update downloadRepositoryIndex to conform to new indexer semantics
The maven indexer semantics were changed to fail if an incremental only
update was requested, but not possible. Previously, it would move on to
a full index update if an incremental failed. downloadRepositoryIndex
needed to be updated to account for this. To keep the behavior the same,
downloadRepositoryIndex will handle a failed incremental only update,
and try a full index update.
04385a1
@ifedorenko

+1

Please merge this if the feature build is successful

@scarlucci scarlucci merged commit ec06047 into master
@scarlucci scarlucci deleted the NXCM-4867-indexer-sync-simple branch
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Apr 11, 2013
  1. @scarlucci
  2. @scarlucci
  3. @scarlucci

    Update maven indexer version

    scarlucci authored
  4. @scarlucci
Commits on Apr 30, 2013
  1. @scarlucci

    Switch to new pseudo-release of maven indexer

    scarlucci authored
    Switch to new pseudo-release of maven indexer, which adds support for
    running an incremental-only index update
  2. @scarlucci

    Merge remote-tracking branch 'origin/master' into NXCM-4867-indexer-s…

    scarlucci authored
    …ync-simple
    
    Conflicts:
    	plugins/indexer/nexus-indexer-lucene-plugin/pom.xml
    	plugins/indexer/nexus-indexer-lucene-plugin/src/main/java/org/sonatype/nexus/index/DefaultIndexerManager.java
  3. @scarlucci

    Change inner class to static

    scarlucci authored
    Inner class does not need access to outer class
Commits on May 3, 2013
  1. @scarlucci

    Update downloadRepositoryIndex to conform to new indexer semantics

    scarlucci authored
    The maven indexer semantics were changed to fail if an incremental only
    update was requested, but not possible. Previously, it would move on to
    a full index update if an incremental failed. downloadRepositoryIndex
    needed to be updated to account for this. To keep the behavior the same,
    downloadRepositoryIndex will handle a failed incremental only update,
    and try a full index update.
  2. @scarlucci
This page is out of date. Refresh to see the latest.
View
3  plugins/indexer/nexus-indexer-lucene-plugin/pom.xml
@@ -30,8 +30,7 @@
<properties>
<pluginName>Nexus Indexer Lucene Plugin</pluginName>
<pluginDescription>Adds search capabilities for repository content.</pluginDescription>
-
- <mavenIndexer.version>5.1.2-c65990c</mavenIndexer.version>
+ <mavenIndexer.version>5.1.2-bf3d48b</mavenIndexer.version>
@kellyrob99 Owner

Just out of curiousity, why the strange version number?

@cstamas Owner
cstamas added a note

These are "pseudo" releases of ASF Maven Indexer. ASF MI 5.1.1 was released last, codebase is currently on 5.1.2-SNAPSHOT. The "bf3d48b" suffix is basically the Git sha1 commit hash out of which the pseudo release of MI was done.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
<aether.version>1.13.1</aether.version>
</properties>
View
187 ...s-indexer-lucene-plugin/src/main/java/org/sonatype/nexus/index/DefaultIndexerManager.java
@@ -1060,47 +1060,35 @@ private void reindexRepository( final Repository repository, final String fromPa
{
logger.debug( "Reindexing repository {} fromPath={} fullReindex={}", repository.getId(), fromPath,
fullReindex );
-
- Runnable runnable = new Runnable()
+
+ if ( !fullReindex )
{
- @Override
- public void run( final IndexingContext context )
- throws IOException
+ //Try incremental update first
+ try
{
- if ( ISPROXY( repository ) )
- {
- updateRemoteIndex( repository.adaptToFacet( ProxyRepository.class ), context, fullReindex );
- }
-
- TaskUtil.checkInterruption();
-
- // igorf, this needs be merged back to maven indexer, see MINDEXER-65
- final IndexSearcher contextIndexSearcher = context.acquireIndexSearcher();
- try
- {
- final NexusScanningListener scanListener =
- new NexusScanningListener( context, contextIndexSearcher, fullReindex,
- ISPROXY( repository ) );
- scanner.scan( new ScanningRequest( context, scanListener, fromPath ) );
- }
- finally
- {
- context.releaseIndexSearcher( contextIndexSearcher );
- }
+ Runnable runnable = new IndexUpdateRunnable(repository, fromPath, false);
+ sharedSingle( repository, runnable );
+ logger.debug( "Reindexed repository {}", repository.getId() );
+ return;
+ }
+ catch(IncrementalIndexUpdateException e)
+ {
+ //This exception is an indication that an incremental
+ //update is not possible, and a full update is necessary
+ logger.info( "Unable to incrementally update index for repository {}. Trying full index update", repository.getId() );
+
+ //Let execution continue to below to try full index update
}
- };
- if ( fullReindex )
- {
- // delete published stuff too, as we are breaking incremental downstream chain
- deleteIndexItems( repository );
- // creates a temp ctx and finally replaces the "real" with temp
- temporary( repository, runnable );
- }
- else
- {
- // scans directly into "real" ctx
- sharedSingle( repository, runnable );
}
+
+ //Perform full index update
+ Runnable runnable = new IndexUpdateRunnable(repository, fromPath, true);
+
+ // delete published stuff too, as we are breaking incremental downstream chain
+ deleteIndexItems( repository );
+
+ // creates a temp ctx and finally replaces the "real" with temp
+ temporary( repository, runnable );
logger.debug( "Reindexed repository {}", repository.getId() );
}
@@ -1116,6 +1104,49 @@ public void run( final IndexingContext context )
repository.getId() );
}
}
+
+ /**
+ * Runnable implementation for updating an index
+ */
+ private class IndexUpdateRunnable implements Runnable
+ {
+ Repository repository;
+ String fromPath;
+ boolean fullReindex;
+
+ IndexUpdateRunnable(Repository repository, String fromPath, boolean fullReindex)
+ {
+ this.repository = repository;
+ this.fromPath = fromPath;
+ this.fullReindex = fullReindex;
+ }
+
+ @Override
+ public void run( final IndexingContext context )
+ throws IOException
+ {
+ if ( ISPROXY( this.repository ) )
+ {
+ updateRemoteIndex( this.repository.adaptToFacet( ProxyRepository.class ), context, this.fullReindex );
+ }
+
+ TaskUtil.checkInterruption();
+
+ // igorf, this needs be merged back to maven indexer, see MINDEXER-65
+ final IndexSearcher contextIndexSearcher = context.acquireIndexSearcher();
+ try
+ {
+ final NexusScanningListener scanListener =
+ new NexusScanningListener( context, contextIndexSearcher, this.fullReindex,
+ ISPROXY( this.repository ) );
+ scanner.scan( new ScanningRequest( context, scanListener, this.fromPath ) );
+ }
+ finally
+ {
+ context.releaseIndexSearcher( contextIndexSearcher );
+ }
+ }
+ }
// ----------------------------------------------------------------------------
// Downloading remote indexes (will do remote-download, merge only)
@@ -1199,23 +1230,48 @@ protected void downloadRepositoryIndex( final ProxyRepository repository, final
{
try
{
+ if ( !forceFullUpdate )
+ {
+ //Try incremental update first
+ try
+ {
+ Runnable runnable = new Runnable()
+ {
+ @Override
+ public void run( IndexingContext context )
+ throws IOException
+ {
+ updateRemoteIndex( repository, context, false );
+ }
+ };
+
+ sharedSingle( repository, runnable );
+ return;
+ }
+ catch(IncrementalIndexUpdateException e)
+ {
+ //This exception is an indication that an incremental
+ //update is not possible, and a full update is necessary
+ logger.info( "Unable to incrementally update index for repository {}. Trying full index update", repository.getId() );
+
+ //Let execution continue to below to try full index update
+ }
+ }
+
+ //If we're here, either a full update was requested, or incremental failed
+ //Try full index update
+
Runnable runnable = new Runnable()
{
@Override
public void run( IndexingContext context )
throws IOException
{
- updateRemoteIndex( repository, context, forceFullUpdate );
+ updateRemoteIndex( repository, context, true );
}
};
- if ( forceFullUpdate )
- {
- temporary( repository, runnable );
- }
- else
- {
- sharedSingle( repository, runnable );
- }
+
+ temporary( repository, runnable );
}
finally
{
@@ -1308,7 +1364,18 @@ public InputStream retrieve( String name )
}
} );
- updateRequest.setForceFullUpdate( forceFullUpdate );
+ //Set request for either full or incremental-only update
+ if( forceFullUpdate )
+ {
+ updateRequest.setForceFullUpdate( true );
+ updateRequest.setIncrementalOnly( false );
+ }
+ else
+ {
+ updateRequest.setForceFullUpdate( false );
+ updateRequest.setIncrementalOnly( true );
+ }
+
updateRequest.setFSDirectoryFactory( luceneDirectoryFactory );
if ( repository instanceof MavenRepository )
@@ -1322,6 +1389,14 @@ public InputStream retrieve( String name )
{
IndexUpdateResult result = indexUpdater.fetchAndUpdateIndex( updateRequest );
+ //Check if successful
+ if( !result.isSuccessful() )
+ {
+ //This condition occurs when we have requested an incremental-only update,
+ //but it could not be completed. In this case, we need to request a full update
+ //This needs to be communicated upstream so that the proper locking can take place
+ throw new IncrementalIndexUpdateException("Cannot incrementally update index. Request a full update");
+ }
boolean hasRemoteIndexUpdate = result.getTimestamp() != null;
if ( hasRemoteIndexUpdate )
@@ -1355,6 +1430,14 @@ public InputStream retrieve( String name )
logger.warn( RepositoryStringUtils.getFormattedMessage(
"Cannot fetch remote index for repository %s, task cancelled.", repository ) );
}
+ catch ( IncrementalIndexUpdateException e )
+ {
+ //This is an indication that an incremental index update is not possible, and a full index
+ //update must be performed.
+ //Just log this, and pass this exception upstream so that it can be handled appropriately
+ logger.info( "Cannot incrementally update index for repository {}", repository.getId() );
+ throw e;
+ }
catch ( IOException e )
{
logger.warn( RepositoryStringUtils.getFormattedMessage(
@@ -2800,4 +2883,14 @@ public IndexingContext getRepositoryIndexContext( String repositoryId )
{
return mavenIndexer.getIndexingContexts().get( getContextId( repositoryId ) );
}
+

I recommend changing exception class name to IndexUpdateException. Please also mark the class as static as it does not and should not need access any member of the outer class.

Changed the class to be static. Left the name to be as explicit as possible about the conditions under which this exception is used, but can be convinced to change if someone has a strong opinion about this name

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
+ private static class IncrementalIndexUpdateException extends IOException
+ {
+ private static final long serialVersionUID = 6444842181110866037L;
+
+ public IncrementalIndexUpdateException(String message)
+ {
+ super(message);
+ }
+ }
}
Something went wrong with that request. Please try again.