From a431474b600b16bdd53e43ab5d950ae8e2fe74ae Mon Sep 17 00:00:00 2001 From: Jon Marius Venstad Date: Sun, 14 Feb 2021 12:19:57 +0100 Subject: Revert "Close the underlying Curator to avoid leaking threads and other resources." This reverts commit fb0524c28864e0311e819cc73a5f6b8f2a6e8b60. --- .../src/main/java/ai/vespa/reindexing/Reindexer.java | 19 ++++++------------- .../java/ai/vespa/reindexing/ReindexingCurator.java | 7 +------ .../ai/vespa/reindexing/ReindexingMaintainer.java | 9 ++------- .../test/java/ai/vespa/reindexing/ReindexerTest.java | 8 +------- 4 files changed, 10 insertions(+), 33 deletions(-) diff --git a/clustercontroller-reindexer/src/main/java/ai/vespa/reindexing/Reindexer.java b/clustercontroller-reindexer/src/main/java/ai/vespa/reindexing/Reindexer.java index 2c34c3164aa..af43211011a 100644 --- a/clustercontroller-reindexer/src/main/java/ai/vespa/reindexing/Reindexer.java +++ b/clustercontroller-reindexer/src/main/java/ai/vespa/reindexing/Reindexer.java @@ -38,7 +38,7 @@ import static java.util.logging.Level.WARNING; * * @author jonmv */ -public class Reindexer implements AutoCloseable { +public class Reindexer { private static final Logger log = Logger.getLogger(Reindexer.class.getName()); @@ -81,6 +81,11 @@ public class Reindexer implements AutoCloseable { this.clock = clock; } + /** Lets the reindexer abort any ongoing visit session, wait for it to complete normally, then exit. */ + public void shutdown() { + phaser.forceTermination(); // All parties waiting on this phaser are immediately allowed to proceed. + } + /** Starts and tracks reprocessing of ready document types until done, or interrupted. */ public void reindex() throws ReindexingLockException { if (phaser.isTerminated()) @@ -206,18 +211,6 @@ public class Reindexer implements AutoCloseable { return parameters; } - @Override - public void close() throws Exception { - phaser.forceTermination(); // All parties waiting on this phaser are immediately allowed to proceed. - database.close(); - } - - @Override - public String toString() { - return "Reindexer{" + - "cluster=" + cluster + - '}'; - } static class Cluster { diff --git a/clustercontroller-reindexer/src/main/java/ai/vespa/reindexing/ReindexingCurator.java b/clustercontroller-reindexer/src/main/java/ai/vespa/reindexing/ReindexingCurator.java index f3edf30f3b2..22ae54fcc6b 100644 --- a/clustercontroller-reindexer/src/main/java/ai/vespa/reindexing/ReindexingCurator.java +++ b/clustercontroller-reindexer/src/main/java/ai/vespa/reindexing/ReindexingCurator.java @@ -26,7 +26,7 @@ import static java.util.stream.Collectors.toUnmodifiableMap; * * @author jonmv */ -public class ReindexingCurator implements AutoCloseable { +public class ReindexingCurator { private final Curator curator; private final ReindexingSerializer serializer; @@ -65,11 +65,6 @@ public class ReindexingCurator implements AutoCloseable { private Path statusPath(String clusterName) { return rootPath(clusterName).append("status"); } private Path lockPath(String clusterName) { return rootPath(clusterName).append("lock"); } - @Override - public void close() throws Exception { - curator.close(); - } - private static class ReindexingSerializer { diff --git a/clustercontroller-reindexer/src/main/java/ai/vespa/reindexing/ReindexingMaintainer.java b/clustercontroller-reindexer/src/main/java/ai/vespa/reindexing/ReindexingMaintainer.java index 101989c6d17..a37a24d2b01 100644 --- a/clustercontroller-reindexer/src/main/java/ai/vespa/reindexing/ReindexingMaintainer.java +++ b/clustercontroller-reindexer/src/main/java/ai/vespa/reindexing/ReindexingMaintainer.java @@ -98,13 +98,8 @@ public class ReindexingMaintainer extends AbstractComponent { @Override public void deconstruct() { try { - for (Reindexer reindexer : reindexers) { - try { - reindexer.close(); - } catch (Exception e) { - log.log(WARNING, "Received exception while closing down reindexer " + reindexer.toString() + " : ", e); - } - } + for (Reindexer reindexer : reindexers) + reindexer.shutdown(); executor.shutdown(); if ( ! executor.awaitTermination(45, TimeUnit.SECONDS)) diff --git a/clustercontroller-reindexer/src/test/java/ai/vespa/reindexing/ReindexerTest.java b/clustercontroller-reindexer/src/test/java/ai/vespa/reindexing/ReindexerTest.java index c5b72db4dee..8cf96fa8c45 100644 --- a/clustercontroller-reindexer/src/test/java/ai/vespa/reindexing/ReindexerTest.java +++ b/clustercontroller-reindexer/src/test/java/ai/vespa/reindexing/ReindexerTest.java @@ -28,12 +28,10 @@ import java.util.concurrent.Executors; import java.util.concurrent.atomic.AtomicBoolean; import java.util.concurrent.atomic.AtomicReference; -import static java.util.logging.Level.WARNING; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertSame; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; -import static org.junit.jupiter.api.Assertions.fail; /** * @author jonmv @@ -142,11 +140,7 @@ class ReindexerTest { aborted.set(new Reindexer(cluster, Map.of(music, Instant.ofEpochMilli(20)), database, parameters -> { database.writeReindexing(Reindexing.empty(), "cluster"); // Wipe database to verify we write data from reindexer. parameters.getControlHandler().onProgress(new ProgressToken()); - try { - aborted.get().close(); - } catch (Exception e) { - fail(e); - } + aborted.get().shutdown(); return () -> { shutDown.set(true); parameters.getControlHandler().onDone(VisitorControlHandler.CompletionCode.ABORTED, "Shut down"); -- cgit v1.2.3 From 2d4809f59c0948bc4aa92eb3a3eddb7fdb9e8010 Mon Sep 17 00:00:00 2001 From: Jon Marius Venstad Date: Sun, 14 Feb 2021 12:20:47 +0100 Subject: Shut down shared Curator used by Reindexers --- .../src/main/java/ai/vespa/reindexing/ReindexingMaintainer.java | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/clustercontroller-reindexer/src/main/java/ai/vespa/reindexing/ReindexingMaintainer.java b/clustercontroller-reindexer/src/main/java/ai/vespa/reindexing/ReindexingMaintainer.java index a37a24d2b01..4e263fb865d 100644 --- a/clustercontroller-reindexer/src/main/java/ai/vespa/reindexing/ReindexingMaintainer.java +++ b/clustercontroller-reindexer/src/main/java/ai/vespa/reindexing/ReindexingMaintainer.java @@ -50,6 +50,7 @@ public class ReindexingMaintainer extends AbstractComponent { private static final Logger log = Logger.getLogger(Reindexing.class.getName()); + private final Curator curator; private final List reindexers; private final ScheduledExecutorService executor; @@ -65,11 +66,12 @@ public class ReindexingMaintainer extends AbstractComponent { ReindexingMaintainer(Clock clock, Metric metric, DocumentAccess access, ZookeepersConfig zookeepersConfig, ClusterListConfig clusterListConfig, AllClustersBucketSpacesConfig allClustersBucketSpacesConfig, ReindexingConfig reindexingConfig) { + this.curator = Curator.create(zookeepersConfig.zookeeperserverlist()); + ReindexingCurator reindexingCurator = new ReindexingCurator(curator, access.getDocumentTypeManager()); this.reindexers = reindexingConfig.clusters().entrySet().stream() .map(cluster -> new Reindexer(parseCluster(cluster.getKey(), clusterListConfig, allClustersBucketSpacesConfig, access.getDocumentTypeManager()), parseReady(cluster.getValue(), access.getDocumentTypeManager()), - new ReindexingCurator(Curator.create(zookeepersConfig.zookeeperserverlist()), - access.getDocumentTypeManager()), + reindexingCurator, access, metric, clock)) @@ -111,6 +113,8 @@ public class ReindexingMaintainer extends AbstractComponent { } if ( ! executor.isShutdown()) executor.shutdownNow(); + + curator.close(); } static Map parseReady(ReindexingConfig.Clusters cluster, DocumentTypeManager manager) { -- cgit v1.2.3 From 693874c67d1500f00d9388f1e71f799cfe4e3cad Mon Sep 17 00:00:00 2001 From: Jon Marius Venstad Date: Sun, 14 Feb 2021 13:28:08 +0100 Subject: Also shut down Curator instances created during clustercontroller component setup --- .../clustercontroller/apps/clustercontroller/ClusterController.java | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/clustercontroller-apps/src/main/java/com/yahoo/vespa/clustercontroller/apps/clustercontroller/ClusterController.java b/clustercontroller-apps/src/main/java/com/yahoo/vespa/clustercontroller/apps/clustercontroller/ClusterController.java index ff7cbbf83da..8ec93b99aff 100644 --- a/clustercontroller-apps/src/main/java/com/yahoo/vespa/clustercontroller/apps/clustercontroller/ClusterController.java +++ b/clustercontroller-apps/src/main/java/com/yahoo/vespa/clustercontroller/apps/clustercontroller/ClusterController.java @@ -100,8 +100,9 @@ public class ClusterController extends AbstractComponent */ private void verifyThatZooKeeperWorks(FleetControllerOptions options) throws Exception { if (options.zooKeeperServerAddress != null && !"".equals(options.zooKeeperServerAddress)) { - Curator curator = Curator.create(options.zooKeeperServerAddress); - curator.framework().blockUntilConnected(); + try (Curator curator = Curator.create(options.zooKeeperServerAddress)) { + curator.framework().blockUntilConnected(); + } } } -- cgit v1.2.3 From 91c7af56b20a0d93a0db550cecca2059a7665992 Mon Sep 17 00:00:00 2001 From: Jon Marius Venstad Date: Sun, 14 Feb 2021 13:30:17 +0100 Subject: Add TODO for Provider && gp --- zkfacade/src/main/java/com/yahoo/vespa/curator/Curator.java | 1 + 1 file changed, 1 insertion(+) diff --git a/zkfacade/src/main/java/com/yahoo/vespa/curator/Curator.java b/zkfacade/src/main/java/com/yahoo/vespa/curator/Curator.java index 90eec5760fc..adfd9bd051f 100644 --- a/zkfacade/src/main/java/com/yahoo/vespa/curator/Curator.java +++ b/zkfacade/src/main/java/com/yahoo/vespa/curator/Curator.java @@ -78,6 +78,7 @@ public class Curator implements VespaCurator, AutoCloseable { } @Inject + // TODO jonmv: Use a Provider for this, due to required shutdown. public Curator(CuratorConfig curatorConfig, @SuppressWarnings("unused") VespaZooKeeperServer server) { // Depends on ZooKeeperServer to make sure it is started first this(ConnectionSpec.create(curatorConfig.server(), -- cgit v1.2.3