Uploaded image for project: 'Jackrabbit Oak'
  1. Jackrabbit Oak
  2. OAK-8512

Without prefetch CopyOnRead opens index files remotely even if they are locally available

Attach filesAttach ScreenshotAdd voteVotersWatch issueWatchersCreate sub-taskLinkCloneUpdate Comment AuthorReplace String in CommentUpdate Comment VisibilityDelete Comments
    XMLWordPrintableJSON

    Details

    • Type: Bug
    • Status: Open
    • Priority: Minor
    • Resolution: Unresolved
    • Affects Version/s: None
    • Fix Version/s: None
    • Component/s: lucene
    • Labels:
      None

      Description

      Without prefetch enabled, CopyOnReadDirectory schedules a copy of file when Directory#openInput is invoked. This happens even if there might a copy of file completely available already (maybe due to CopyOnWriteDirectory). While the scheduled copy would check if the file is valid already but by that time often index file are accessed remotely.

      The fix is fairly simple - don't schedule if file exists locally and passes our weak test that length matches what we expect [0].

      But, since we strongly advise to enable prefetch, the priority of the issue is minor. Perf impact would be significant during application start so sev should high I guess.

      [0]:

      $ git diff oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/directory/CopyOnReadDirectory.java
      diff --git a/oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/directory/CopyOnReadDirectory.java b/oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/directory/CopyOnReadDirectory.java
      index b6b16286d2..d49e5d9ea7 100644
      --- a/oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/directory/CopyOnReadDirectory.java
      +++ b/oak-lucene/src/main/java/org/apache/jackrabbit/oak/plugins/index/lucene/directory/CopyOnReadDirectory.java
      @@ -126,11 +126,12 @@ public class CopyOnReadDirectory extends FilterDirectory {
               CORFileReference toPut = new CORFileReference(name);
               CORFileReference old = files.putIfAbsent(name, toPut);
               if (old == null) {
      -            log.trace("[{}] scheduled local copy for {}", indexPath, name);
      -            copy(toPut);
      +            if (copy(toPut)) {
      +                log.trace("[{}] scheduled local copy for {}", indexPath, name);
      +            }
               }
       
      -        //If immediate executor is used the result would be ready right away
      +        //If immediate executor is used OR local file already existed, the result would be ready right away
               if (toPut.isLocalValid()) {
                   log.trace("[{}] opening new local file {}", indexPath, name);
                   return toPut.openLocalInput(context);
      @@ -145,7 +146,21 @@ public class CopyOnReadDirectory extends FilterDirectory {
               return local;
           }
       
      -    private void copy(final CORFileReference reference) {
      +    private boolean copy(final CORFileReference reference) {
      +        // quick sanity check
      +        if (reference.isLocalValid()) {
      +            return false;
      +        }
      +        try {
      +            String name = reference.name;
      +            if (local.fileExists(name) && local.fileLength(name) == remote.fileLength(name)) {
      +                reference.markValid();
      +                return false;
      +            }
      +        } catch (IOException e) {
      +            // ignore
      +        }
      +
               indexCopier.scheduledForCopy();
               executor.execute(new Runnable() {
                   @Override
      @@ -154,6 +169,8 @@ public class CopyOnReadDirectory extends FilterDirectory {
                       copyFilesToLocal(reference, true, true);
                   }
               });
      +
      +        return true;
           }
      

        Attachments

        Issue Links

          Activity

            People

            • Assignee:
              Unassigned
              Reporter:
              catholicon Vikas Saurabh

              Dates

              • Created:
                Updated:

                Issue deployment