Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve exception handling and logging in baremaps #770

Closed
bchapuis opened this issue Sep 2, 2023 · 1 comment
Closed

Improve exception handling and logging in baremaps #770

bchapuis opened this issue Sep 2, 2023 · 1 comment

Comments

@bchapuis
Copy link
Member

bchapuis commented Sep 2, 2023

I'd like to improve the way we trigger and handle exception in baremaps. At the moment, baremaps mostly use checked exceptions. This is problematic because we extensively rely on the Stream API which does not support checked exception. Therefore, we may be tempted to introduce the folling kind of exception caching and rethrowing code in the codebase (pseudo-code):

try {
  stream.map(elem -> {
    try {
        // do something
    } catch(BaremapsException e) {
       throw new RuntimeException(e);
    }
  }).toList();
} catch (RuntimeException e) {
    if (e.getCause() instanceof BaremapsException) {
      throw e.getCause();
    } else {
       throw e;
    }
}

In addition to being unreadable, it is easy to forget to log exceptions correctly as reported in #763.

My current opinion is that we should use uncatched exceptions instead of checked exception in the API and let the error propagate several levels up. We should then systematically catch and log them at the level of the workflow executor. In addition to being more readable and reducing the amount code related to exception handling, this approach may enable us to build comprehensive execution reports.

@Perdjesk @polastre What do you think?

@cmahnke
Copy link

cmahnke commented Sep 14, 2023

This is also required to stop a workflow run, if there is an error in a previous step:


[INFO ] 2023-09-14 11:27:58.400 [main] Execute - Executing the workflow workflow.json
[INFO ] 2023-09-14 11:27:58.529 [pool-2-thread-1] DownloadUrl - Downloading https://download.geofabrik.de/europe/liechtenstein-latest.osm.pbf to liechtenstein-latest.osm.pbf
[INFO ] 2023-09-14 11:27:58.895 [pool-2-thread-1] DownloadUrl - Skipping download of https://download.geofabrik.de/europe/liechtenstein-latest.osm.pbf to liechtenstein-latest.osm.pbf
[INFO ] 2023-09-14 11:27:58.895 [pool-2-thread-2] ImportOpenStreetMap - Importing liechtenstein-latest.osm.pbf into jdbc:postgresql://localhost:5432/baremaps?&user=baremaps&password=baremaps
[INFO ] 2023-09-14 11:27:58.902 [pool-2-thread-2] HikariDataSource - HikariPool-1 - Starting...
[INFO ] 2023-09-14 11:27:59.107 [pool-2-thread-2] HikariPool - HikariPool-1 - Added connection org.postgresql.jdbc.PgConnection@e74ca58
[INFO ] 2023-09-14 11:27:59.108 [pool-2-thread-2] HikariDataSource - HikariPool-1 - Start completed.
[WARN ] 2023-09-14 11:28:01.593 [ForkJoinPool.commonPool-worker-6] RelationGeometryBuilder - Unable to build the geometry for relation #5401257
org.apache.baremaps.stream.StreamException: java.lang.NullPointerException: Cannot invoke "java.util.List.stream()" because "refs" is null
	at org.apache.baremaps.openstreetmap.function.RelationGeometryBuilder.createLine(RelationGeometryBuilder.java:168) ~[baremaps-core-0.7.1.jar:?]
	at org.apache.baremaps.openstreetmap.function.RelationGeometryBuilder.lambda$createPolygons$2(RelationGeometryBuilder.java:143) ~[baremaps-core-0.7.1.jar:?]
	at java.util.stream.ForEachOps$ForEachOp$OfRef.accept(ForEachOps.java:183) ~[?:?]
	at java.util.stream.ReferencePipeline$2$1.accept(ReferencePipeline.java:179) ~[?:?]
	at java.util.stream.ReferencePipeline$2$1.accept(ReferencePipeline.java:179) ~[?:?]
	at java.util.ArrayList$ArrayListSpliterator.forEachRemaining(ArrayList.java:1625) ~[?:?]
	at java.util.stream.AbstractPipeline.copyInto(AbstractPipeline.java:509) ~[?:?]
	at java.util.stream.AbstractPipeline.wrapAndCopyInto(AbstractPipeline.java:499) ~[?:?]
	at java.util.stream.ForEachOps$ForEachOp.evaluateSequential(ForEachOps.java:150) ~[?:?]
	at java.util.stream.ForEachOps$ForEachOp$OfRef.evaluateSequential(ForEachOps.java:173) ~[?:?]
	at java.util.stream.AbstractPipeline.evaluate(AbstractPipeline.java:234) ~[?:?]
	at java.util.stream.ReferencePipeline.forEach(ReferencePipeline.java:596) ~[?:?]
	at org.apache.baremaps.openstreetmap.function.RelationGeometryBuilder.createPolygons(RelationGeometryBuilder.java:142) ~[baremaps-core-0.7.1.jar:?]
	at org.apache.baremaps.openstreetmap.function.RelationGeometryBuilder.accept(RelationGeometryBuilder.java:71) ~[baremaps-core-0.7.1.jar:?]
	at org.apache.baremaps.openstreetmap.function.EntityGeometryBuilder.accept(EntityGeometryBuilder.java:54) ~[baremaps-core-0.7.1.jar:?]
	at org.apache.baremaps.openstreetmap.function.EntityGeometryBuilder.accept(EntityGeometryBuilder.java:27) ~[baremaps-core-0.7.1.jar:?]
	at java.util.function.Consumer.lambda$andThen$0(Consumer.java:65) ~[?:?]
	at java.util.function.Consumer.lambda$andThen$0(Consumer.java:65) ~[?:?]
	at java.util.ArrayList.forEach(ArrayList.java:1511) ~[?:?]
	at org.apache.baremaps.openstreetmap.function.BlockEntitiesHandler.accept(BlockEntitiesHandler.java:47) ~[baremaps-core-0.7.1.jar:?]
	at org.apache.baremaps.openstreetmap.function.BlockEntitiesHandler.accept(BlockEntitiesHandler.java:25) ~[baremaps-core-0.7.1.jar:?]
	at org.apache.baremaps.stream.ConsumerUtils.lambda$consumeThenReturn$1(ConsumerUtils.java:46) ~[baremaps-core-0.7.1.jar:?]
	at java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:197) ~[?:?]
	at java.util.stream.ReferencePipeline$3$1.accept(ReferencePipeline.java:197) ~[?:?]
	at org.apache.baremaps.stream.BufferedSpliterator.tryAdvance(BufferedSpliterator.java:74) ~[baremaps-core-0.7.1.jar:?]
	at java.util.stream.StreamSpliterators$WrappingSpliterator.lambda$initPartialTraversalState$0(StreamSpliterators.java:292) ~[?:?]
	at java.util.stream.StreamSpliterators$AbstractWrappingSpliterator.fillBuffer(StreamSpliterators.java:206) ~[?:?]
	at java.util.stream.StreamSpliterators$AbstractWrappingSpliterator.doAdvance(StreamSpliterators.java:169) ~[?:?]
	at java.util.stream.StreamSpliterators$WrappingSpliterator.tryAdvance(StreamSpliterators.java:298) ~[?:?]
	at org.apache.baremaps.stream.BatchedSpliterator.tryAdvance(BatchedSpliterator.java:48) ~[baremaps-core-0.7.1.jar:?]
	at org.apache.baremaps.stream.BatchedSpliterator.trySplit(BatchedSpliterator.java:59) ~[baremaps-core-0.7.1.jar:?]
	at java.util.stream.ForEachOps$ForEachTask.compute(ForEachOps.java:289) ~[?:?]
	at java.util.concurrent.CountedCompleter.exec(CountedCompleter.java:754) ~[?:?]
	at java.util.concurrent.ForkJoinTask.doExec(ForkJoinTask.java:387) ~[?:?]
	at java.util.concurrent.ForkJoinPool$WorkQueue.topLevelExec(ForkJoinPool.java:1312) ~[?:?]
	at java.util.concurrent.ForkJoinPool.scan(ForkJoinPool.java:1843) ~[?:?]
	at java.util.concurrent.ForkJoinPool.runWorker(ForkJoinPool.java:1808) ~[?:?]
	at java.util.concurrent.ForkJoinWorkerThread.run(ForkJoinWorkerThread.java:188) ~[?:?]
Caused by: java.lang.NullPointerException: Cannot invoke "java.util.List.stream()" because "refs" is null
	at org.apache.baremaps.openstreetmap.function.RelationGeometryBuilder.createLine(RelationGeometryBuilder.java:164) ~[baremaps-core-0.7.1.jar:?]
	... 37 more
[INFO ] 2023-09-14 11:28:01.970 [pool-2-thread-2] ImportOpenStreetMap - Finished importing liechtenstein-latest.osm.pbf into jdbc:postgresql://localhost:5432/baremaps?&user=baremaps&password=baremaps
[INFO ] 2023-09-14 11:28:01.971 [pool-2-thread-3] ExecuteSql - Executing indexes.sql

And yes: This means that the current documented example is broken with 0.7.1

Someone already spotted this issue: #743 (comment)

[Update]
This is fixed in the current Git version.

@bchapuis bchapuis closed this as completed Nov 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants