more PR feedback

onthegomap · Jan 2, 2024 · 148ba33 · 148ba33
1 parent a280684
commit 148ba33
Show file tree

Hide file tree

Showing 10 changed files with 225 additions and 67 deletions.
diff --git a/planetiler-core/src/main/java/com/onthegomap/planetiler/archive/TileArchiveWriter.java b/planetiler-core/src/main/java/com/onthegomap/planetiler/archive/TileArchiveWriter.java
@@ -402,7 +402,6 @@ private long tilesEmitted() {
 
   private void finishArchive() {
     archive.finish(tileArchiveMetadata.withLayerStats(layerAttrStats.getTileStats()));
-    archive.printStats();
   }
 
   /**

diff --git a/planetiler-core/src/main/java/com/onthegomap/planetiler/archive/WriteableTileArchive.java b/planetiler-core/src/main/java/com/onthegomap/planetiler/archive/WriteableTileArchive.java
@@ -44,8 +44,6 @@ default void initialize() {}
    */
   default void finish(TileArchiveMetadata tileArchiveMetadata) {}
 
-  default void printStats() {}
-
   long bytesWritten();
 
   interface TileWriter extends Closeable {

diff --git a/planetiler-core/src/main/java/com/onthegomap/planetiler/files/FilesArchiveUtils.java b/planetiler-core/src/main/java/com/onthegomap/planetiler/files/FilesArchiveUtils.java
@@ -7,6 +7,7 @@
 import com.onthegomap.planetiler.config.Arguments;
 import java.nio.file.Path;
 import java.nio.file.Paths;
+import java.util.Objects;
 import java.util.Optional;
 
 public final class FilesArchiveUtils {
@@ -45,29 +46,48 @@ static TileSchemeEncoding tilesSchemeEncoding(Arguments options, Path basePath,
   }
 
   static BasePathWithTileSchemeEncoding basePathWithTileSchemeEncoding(Arguments options, Path basePath) {
-    final String basePathStr = basePath.toString();
-    final int curlyIndex = basePathStr.indexOf('{');
-    if (curlyIndex >= 0) {
-      final Path newBasePath = Paths.get(basePathStr.substring(0, curlyIndex));
-      return new BasePathWithTileSchemeEncoding(
-        newBasePath,
-        tilesSchemeEncoding(options, newBasePath, basePathStr.substring(curlyIndex))
-      );
-    } else {
-      return new BasePathWithTileSchemeEncoding(
-        basePath,
-        tilesSchemeEncoding(options, basePath, Path.of(Z_TEMPLATE, X_TEMPLATE, Y_TEMPLATE + ".pbf").toString()));
-    }
+
+    final SplitShortcutPath split = SplitShortcutPath.split(basePath);
+
+    final String tileScheme = Objects
+      .requireNonNullElse(split.tileSchemePart(), Path.of(Z_TEMPLATE, X_TEMPLATE, Y_TEMPLATE + ".pbf")).toString();
+
+    return new BasePathWithTileSchemeEncoding(
+      split.basePart(),
+      tilesSchemeEncoding(options, split.basePart(), tileScheme)
+    );
   }
 
   public static Path cleanBasePath(Path basePath) {
-    final String basePathStr = basePath.toString();
-    final int curlyIndex = basePathStr.indexOf('{');
-    if (curlyIndex >= 0) {
-      return Paths.get(basePathStr.substring(0, curlyIndex));
-    }
-    return basePath;
+    return SplitShortcutPath.split(basePath).basePart();
   }
 
   record BasePathWithTileSchemeEncoding(Path basePath, TileSchemeEncoding tileSchemeEncoding) {}
+
+  private record SplitShortcutPath(Path basePart, Path tileSchemePart) {
+    public static SplitShortcutPath split(Path basePath) {
+      Path basePart = Objects.requireNonNullElse(basePath.getRoot(), Paths.get(""));
+      Path tileSchemePart = null;
+
+      boolean remainingIsTileScheme = false;
+      for (int i = 0; i < basePath.getNameCount(); i++) {
+        final Path part = basePath.getName(i);
+        if (!remainingIsTileScheme && part.toString().contains("{")) {
+          remainingIsTileScheme = true;
+        }
+        if (remainingIsTileScheme) {
+          tileSchemePart = tileSchemePart == null ? part : tileSchemePart.resolve(part);
+        } else {
+          basePart = basePart.resolve(part);
+        }
+      }
+
+      if (tileSchemePart == null) {
+        // just in case: use the "original" basePath in case no tile scheme is included, but basePart _should_ be identical
+        return new SplitShortcutPath(basePath, null);
+      } else {
+        return new SplitShortcutPath(basePart, tileSchemePart);
+      }
+    }
+  }
 }
diff --git a/planetiler-core/src/main/java/com/onthegomap/planetiler/files/TileSchemeEncoding.java b/planetiler-core/src/main/java/com/onthegomap/planetiler/files/TileSchemeEncoding.java
@@ -5,6 +5,7 @@
 import java.io.File;
 import java.nio.file.Path;
 import java.nio.file.Paths;
+import java.util.Objects;
 import java.util.Optional;
 import java.util.function.Function;
 import java.util.regex.Matcher;
@@ -161,4 +162,22 @@ private static String validate(String tileScheme) {
     return tileScheme;
   }
 
+  @Override
+  public boolean equals(Object o) {
+    return this == o || (o instanceof TileSchemeEncoding that && Objects.equals(tileScheme, that.tileScheme) &&
+      Objects.equals(basePath, that.basePath));
+  }
+
+  @Override
+  public int hashCode() {
+    return Objects.hash(tileScheme, basePath);
+  }
+
+  @Override
+  public String toString() {
+    return "TileSchemeEncoding[" +
+      "tileScheme='" + tileScheme + '\'' +
+      ", basePath=" + basePath +
+      ']';
+  }
 }
diff --git a/planetiler-core/src/main/java/com/onthegomap/planetiler/stats/PrometheusStats.java b/planetiler-core/src/main/java/com/onthegomap/planetiler/stats/PrometheusStats.java
@@ -1,6 +1,5 @@
 package com.onthegomap.planetiler.stats;
 
-import com.onthegomap.planetiler.util.FileUtils;
 import com.onthegomap.planetiler.util.MemoryEstimator;
 import io.prometheus.client.Collector;
 import io.prometheus.client.CollectorRegistry;
@@ -49,8 +48,7 @@ class PrometheusStats implements Stats {
   private PushGateway pg;
   private ScheduledExecutorService executor;
   private final String job;
-  private final Map<String, Path> filesToMonitor = new ConcurrentSkipListMap<>();
-  private final Map<String, LongSupplier> sizesOfFilesToMonitor = new ConcurrentSkipListMap<>();
+  private final Map<String, MonitoredFile> filesToMonitor = new ConcurrentSkipListMap<>();
   private final Map<String, Long> dataErrorCounters = new ConcurrentHashMap<>();
   private final Map<String, MemoryEstimator.HasEstimate> heapObjectsToMonitor = new ConcurrentSkipListMap<>();
 
@@ -172,15 +170,10 @@ public Timers timers() {
   }
 
   @Override
-  public Map<String, Path> monitoredFiles() {
+  public Map<String, MonitoredFile> monitoredFiles() {
     return filesToMonitor;
   }
 
-  @Override
-  public Map<String, LongSupplier> monitoredFileSizes() {
-    return sizesOfFilesToMonitor;
-  }
-
   @Override
   public void monitorInMemoryObject(String name, MemoryEstimator.HasEstimate object) {
     heapObjectsToMonitor.put(name, object);
@@ -254,11 +247,11 @@ private class FileSizeCollector extends Collector {
     @Override
     public List<MetricFamilySamples> collect() {
       List<Collector.MetricFamilySamples> results = new ArrayList<>();
-      for (var file : filesToMonitor.entrySet()) {
-        String name = sanitizeMetricName(file.getKey());
-        Path path = file.getValue();
-        var sizeSupplier = monitoredFileSizes().getOrDefault(file.getKey(), () -> FileUtils.size(path));
-        long size = sizeSupplier.getAsLong();
+      for (var entry : filesToMonitor.entrySet()) {
+        String name = sanitizeMetricName(entry.getKey());
+        MonitoredFile monitoredFile = entry.getValue();
+        Path path = monitoredFile.path();
+        long size = monitoredFile.sizeProvider().getAsLong();
         results.add(new GaugeMetricFamily(BASE + "file_" + name + "_size_bytes", "Size of " + name + " in bytes",
           size));
         if (Files.exists(path)) {

diff --git a/planetiler-core/src/main/java/com/onthegomap/planetiler/stats/Stats.java b/planetiler-core/src/main/java/com/onthegomap/planetiler/stats/Stats.java
@@ -60,8 +60,7 @@ default void printSummary() {
       timers().printSummary();
       logger.info("-".repeat(40));
       for (var entry : monitoredFiles().entrySet()) {
-        var sizeSupplier = monitoredFileSizes().getOrDefault(entry.getKey(), () -> FileUtils.size(entry.getValue()));
-        long size = sizeSupplier.getAsLong();
+        long size = entry.getValue().sizeProvider().getAsLong();
         if (size > 0) {
           logger.info("\t{}\t{}B", entry.getKey(), format.storage(size, false));
         }
@@ -119,9 +118,7 @@ default Timers.Finishable startStage(String name, boolean log) {
   Timers timers();
 
   /** Returns all the files being monitored. */
-  Map<String, Path> monitoredFiles();
-
-  Map<String, LongSupplier> monitoredFileSizes();
+  Map<String, MonitoredFile> monitoredFiles();
 
   /** Adds a stat that will track the size of a file or directory located at {@code path}. */
   default void monitorFile(String name, Path path) {
@@ -130,10 +127,7 @@ default void monitorFile(String name, Path path) {
 
   default void monitorFile(String name, Path path, LongSupplier sizeProvider) {
     if (path != null) {
-      monitoredFiles().put(name, path);
-    }
-    if (sizeProvider != null) {
-      monitoredFileSizes().put(name, sizeProvider);
+      monitoredFiles().put(name, new MonitoredFile(path, sizeProvider));
     }
   }
 
@@ -199,7 +193,7 @@ class InMemory implements Stats {
     private InMemory() {}
 
     private final Timers timers = new Timers();
-    private final Map<String, Path> monitoredFiles = new ConcurrentSkipListMap<>();
+    private final Map<String, MonitoredFile> monitoredFiles = new ConcurrentSkipListMap<>();
     private final Map<String, LongSupplier> monitoredFileSizes = new ConcurrentSkipListMap<>();
     private final Map<String, Long> dataErrors = new ConcurrentHashMap<>();
 
@@ -212,15 +206,10 @@ public Timers timers() {
     }
 
     @Override
-    public Map<String, Path> monitoredFiles() {
+    public Map<String, MonitoredFile> monitoredFiles() {
       return monitoredFiles;
     }
 
-    @Override
-    public Map<String, LongSupplier> monitoredFileSizes() {
-      return monitoredFileSizes;
-    }
-
     @Override
     public void monitorInMemoryObject(String name, MemoryEstimator.HasEstimate object) {}
 
@@ -259,4 +248,11 @@ public void close() {
 
     }
   }
+
+  record MonitoredFile(Path path, LongSupplier sizeProvider) {
+    public MonitoredFile(Path path, LongSupplier sizeProvider) {
+      this.path = path;
+      this.sizeProvider = sizeProvider != null ? sizeProvider : () -> FileUtils.size(path);
+    }
+  }
 }
diff --git a/planetiler-core/src/main/java/com/onthegomap/planetiler/stream/WriteableStreamArchive.java b/planetiler-core/src/main/java/com/onthegomap/planetiler/stream/WriteableStreamArchive.java
@@ -47,7 +47,7 @@ abstract class WriteableStreamArchive implements WriteableTileArchive {
 
   private WriteableStreamArchive(OutputStreamSupplier outputStreamFactory, StreamArchiveConfig config) {
     this.outputStreamFactory =
-      i -> new CountingOutputStream(outputStreamFactory.newOutputStream(i), bytesWritten::incBy);
+      i -> new CountingOutputStream(outputStreamFactory.newOutputStream(i), bytesWritten.counterForThread()::incBy);
     this.config = config;
 
     this.primaryOutputStream = this.outputStreamFactory.newOutputStream(0);

diff --git a/planetiler-core/src/test/java/com/onthegomap/planetiler/files/FilesArchiveUtilsTest.java b/planetiler-core/src/test/java/com/onthegomap/planetiler/files/FilesArchiveUtilsTest.java
@@ -0,0 +1,107 @@
+package com.onthegomap.planetiler.files;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+import com.onthegomap.planetiler.config.Arguments;
+import java.nio.file.Path;
+import java.nio.file.Paths;
+import java.util.Map;
+import org.junit.jupiter.api.Test;
+import org.junit.jupiter.api.io.TempDir;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.CsvSource;
+
+class FilesArchiveUtilsTest {
+
+  @ParameterizedTest
+  @CsvSource(textBlock = """
+    {z}/{x}/{y}.pbf           ,      , {z}/{x}/{y}.pbf
+                              ,      , {z}/{x}/{y}.pbf
+    {x}/{y}/{z}.pbf           ,      , {x}/{y}/{z}.pbf
+    tiles/{z}/{x}/{y}.pbf     , tiles, {z}/{x}/{y}.pbf
+    tiles/z{z}/{x}/{y}.pbf    , tiles, z{z}/{x}/{y}.pbf
+    z{z}/x{x}/y{y}.pbf        ,      , z{z}/x{x}/y{y}.pbf
+    tiles/tile-{z}-{x}-{y}.pbf, tiles, tile-{z}-{x}-{y}.pbf
+    /a                        , /a   , {z}/{x}/{y}.pbf
+    /                         , /    , {z}/{x}/{y}.pbf
+    """
+  )
+  void testBasePathWithTileSchemeEncoding(String shortcutBase, String actualBase, String tileScheme,
+    @TempDir Path tempDir) {
+
+    final Path shortcutBasePath = makePath(shortcutBase, tempDir);
+    final Path actualBasePath = makePath(actualBase, tempDir);
+
+    assertEquals(
+      new FilesArchiveUtils.BasePathWithTileSchemeEncoding(
+        actualBasePath,
+        new TileSchemeEncoding(
+          Paths.get(tileScheme).toString(),
+          actualBasePath
+        )
+      ),
+      FilesArchiveUtils.basePathWithTileSchemeEncoding(Arguments.of(), shortcutBasePath)
+    );
+  }
+
+  @Test
+  void testBasePathWithTileSchemeEncodingPrefersArgOverShortcut() {
+    final Path basePath = Paths.get("");
+    final Path schemeShortcutPath = Paths.get("{x}", "{y}", "{z}.pbf");
+    final Path schemeArgumentPath = Paths.get("x{x}", "y{y}", "z{z}.pbf");
+    final Path shortcutPath = basePath.resolve(schemeShortcutPath);
+    assertEquals(
+      new FilesArchiveUtils.BasePathWithTileSchemeEncoding(
+        basePath,
+        new TileSchemeEncoding(
+          schemeShortcutPath.toString(),
+          basePath
+        )
+      ),
+      FilesArchiveUtils.basePathWithTileSchemeEncoding(Arguments.of(), shortcutPath)
+    );
+    assertEquals(
+      new FilesArchiveUtils.BasePathWithTileSchemeEncoding(
+        basePath,
+        new TileSchemeEncoding(
+          schemeArgumentPath.toString(),
+          basePath
+        )
+      ),
+      FilesArchiveUtils.basePathWithTileSchemeEncoding(
+        Arguments.of(Map.of(FilesArchiveUtils.OPTION_TILE_SCHEME, schemeArgumentPath.toString())), shortcutPath)
+    );
+  }
+
+  @ParameterizedTest
+  @CsvSource(textBlock = """
+    {z}/{x}/{y}.pbf           ,
+                              ,
+    {x}/{y}/{z}.pbf           ,
+    tiles/{z}/{x}/{y}.pbf     , tiles
+    tiles/z{z}/{x}/{y}.pbf    , tiles
+    z{z}/x{x}/y{y}.pbf        ,
+    tiles/tile-{z}-{x}-{y}.pbf, tiles
+    /a                        , /a
+    /                         , /
+    """
+  )
+  void testCleanBasePath(String shortcutBase, String actualBase, @TempDir Path tempDir) {
+
+    assertEquals(
+      makePath(actualBase, tempDir),
+      FilesArchiveUtils.cleanBasePath(makePath(shortcutBase, tempDir))
+    );
+  }
+
+
+  private static Path makePath(String in, @TempDir Path tempDir) {
+    if (in == null) {
+      return Paths.get("");
+    }
+    if (in.startsWith("/")) {
+      return tempDir.resolve(in.substring(1));
+    }
+    return Paths.get(in);
+  }
+}