From a16cb89956fffbcc00d50936dd27c4b34c34020b Mon Sep 17 00:00:00 2001
From: Nhat Nguyen <nhat.nguyen@elastic.co>
Date: Wed, 4 Sep 2019 16:50:34 -0400
Subject: [PATCH] Revert "Sync translog without lock when trim unreferenced
 readers (#46203)"

Unfortunately, with this change, we won't clean up all unreferenced
generations when reopening. We assume that there's at most one
unreferenced generation when reopening translog. The previous
implementation guarantees this assumption by syncing translog every time
after we remove a translog reader. This change, however, only syncs
translog once after we have removed all unreferenced readers (can be
more than one) and breaks the assumption.

Closes #46267

This reverts commit fd8183ee51d7cf08d9def58a2ae027714beb60de.
---
 .../index/translog/Translog.java              | 29 ++++++-------------
 1 file changed, 9 insertions(+), 20 deletions(-)

diff --git a/server/src/main/java/org/elasticsearch/index/translog/Translog.java b/server/src/main/java/org/elasticsearch/index/translog/Translog.java
index 8df550f613e..5f7f834c5e2 100644
--- a/server/src/main/java/org/elasticsearch/index/translog/Translog.java
+++ b/server/src/main/java/org/elasticsearch/index/translog/Translog.java
@@ -1678,7 +1678,6 @@ public class Translog extends AbstractIndexShardComponent implements IndexShardC
      * required generation
      */
     public void trimUnreferencedReaders() throws IOException {
-        List<TranslogReader> toDeleteReaderList = new ArrayList<>();
         try (ReleasableLock ignored = writeLock.acquire()) {
             if (closed.get()) {
                 // we're shutdown potentially on some tragic event, don't delete anything
@@ -1692,14 +1691,22 @@ public class Translog extends AbstractIndexShardComponent implements IndexShardC
                 "deletion policy requires a minReferenceGen of [" + minReferencedGen + "] which is higher than the current generation ["
                     + currentFileGeneration() + "]";
 
+
             for (Iterator<TranslogReader> iterator = readers.iterator(); iterator.hasNext(); ) {
                 TranslogReader reader = iterator.next();
                 if (reader.getGeneration() >= minReferencedGen) {
                     break;
                 }
                 iterator.remove();
-                toDeleteReaderList.add(reader);
                 IOUtils.closeWhileHandlingException(reader);
+                final Path translogPath = reader.path();
+                logger.trace("delete translog file [{}], not referenced and not current anymore", translogPath);
+                // The checkpoint is used when opening the translog to know which files should be recovered from.
+                // We now update the checkpoint to ignore the file we are going to remove.
+                // Note that there is a provision in recoverFromFiles to allow for the case where we synced the checkpoint
+                // but crashed before we could delete the file.
+                current.sync();
+                deleteReaderFiles(reader);
             }
             assert readers.isEmpty() == false || current.generation == minReferencedGen :
                 "all readers were cleaned but the minReferenceGen [" + minReferencedGen + "] is not the current writer's gen [" +
@@ -1708,24 +1715,6 @@ public class Translog extends AbstractIndexShardComponent implements IndexShardC
             closeOnTragicEvent(ex);
             throw ex;
         }
-        // Do sync outside the writeLock to avoid blocking all writing thread.
-        if (toDeleteReaderList.isEmpty() == false) {
-            try {
-                // The checkpoint is used when opening the translog to know which files should be recovered from.
-                // We now update the checkpoint to ignore the file we are going to remove.
-                // Note that there is a provision in recoverFromFiles to allow for the case where we synced the checkpoint
-                // but crashed before we could delete the file.
-                sync();
-                for (TranslogReader reader : toDeleteReaderList) {
-                    final Path translogPath = reader.path();
-                    logger.trace("delete translog file [{}], not referenced and not current anymore", translogPath);
-                    deleteReaderFiles(reader);
-                }
-            } catch (final Exception ex) {
-                closeOnTragicEvent(ex);
-                throw ex;
-            }
-        }
     }
 
     /**