From 41d3eb31946a1f6f894fe1c46f57260e2195704e 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 89ee29689c550..305fcf6268912 100644
--- a/server/src/main/java/org/elasticsearch/index/translog/Translog.java
+++ b/server/src/main/java/org/elasticsearch/index/translog/Translog.java
@@ -1670,7 +1670,6 @@ public void rollGeneration() throws IOException {
      * 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
@@ -1684,14 +1683,22 @@ public void trimUnreferencedReaders() throws IOException {
                 "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 [" +
@@ -1700,24 +1707,6 @@ public void trimUnreferencedReaders() throws IOException {
             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;
-            }
-        }
     }
 
     /**