Raw Atlas Flow Integration #25

MikeGost · 2018-04-06T22:24:51Z

~~This build is expected to fail, since it depends on this Atlas PR.~~

The main change here is to integrate the Raw Atlas generation flow and make it backward compatible. This is done by to adding a "useRawAtlas" Flag, which when present will generate an Atlas using the Raw Atlas flow. I've tested this (both with and without) the flag successfully.

Next steps include:

~~Adding Raw Atlas Fetcher for the sectioning step~~
~~Adding Metrics/Statistics for the final Atlas~~
~~General cleanup~~

matthieun

A few requests on the spark calls arrangement, otherwise that looks great!

matthieun · 2018-04-06T22:56:18Z

src/main/java/org/openstreetmap/atlas/generator/AtlasGenerator.java

+                    codeVersion, dataVersion);
+
+            // Slice the raw Atlas and save result
+            final JavaPairRDD<String, Atlas> countrySlicedRawAtlasShardsRDD = sliceAndSaveRawAtlases(


To make it easier to read the Spark flow, it would be nicer to see the "mapToPair" "filter", "reduce" calls here directly.

What I would do instead here is make the generateAndSaveRawAtlas method return a Function that does the transformation for you between the Tuples. That way all you have to do here is something that will look like this:

final JavaPairRDD<String, Atlas> countrySlicedRawAtlasShardsRDD = countryRawAtlasShardsRDD.mapToPair(sliceAndSaveRawAtlases(.....));

Fair point, I'll update this!

matthieun · 2018-04-06T22:57:57Z

src/main/java/org/openstreetmap/atlas/generator/AtlasGenerator.java

                    // Output the Name/Atlas couple
                    final Tuple2<String, Atlas> result = new Tuple2<>(name, atlas);
                    return result;
-                });
+                }).filter(tuple -> tuple._2() != null);


You could also bubble up that filter call at the top level, for the same reasons.

matthieun · 2018-04-06T22:59:00Z

src/main/java/org/openstreetmap/atlas/generator/AtlasGenerator.java

-        final JavaPairRDD<String, Atlas> countryNonNullAtlasShardsRDD = countryAtlasShardsRDD
-                .filter(tuple -> tuple._2() != null);
+        // Persist the RDD to avoid re-computing and save each raw atlas
+        countryRawAtlasShardsRDD.cache().saveAsHadoopFile(


Same, bubble this up at the top level. cache() calls are integral to understanding the Spark flow, and I would love to see them in the top method..

matthieun · 2018-04-06T23:00:10Z

src/main/java/org/openstreetmap/atlas/generator/AtlasGenerator.java

                    try
                    {
-                        statistics = counter.processAtlas(tuple._2());
+                        final AtlasLoadingOption atlasLoadingOption = buildAtlasLoadingOption(


I am not sure I remember how the legacy code passes the AtlasLoadingOption around, but I believe it did not have to do this. We can review this at some point, there might be another way :)

I don't think it passes it around, each slave builds it's own AtlasLoadingOption! Building it once and passing it around didn't work, since it will try to serialize the entire class and its contents - some of which aren't Serializable.

I'll try to think of something a little more elegant, if you have ideas, please let me know!

…elper functions

MikeGost · 2018-04-10T18:38:40Z

This now relies on this Atlas PR.

MikeGost · 2018-04-10T20:36:49Z

This PR is feature complete. It needs a new release of the Atlas project before it can be merged. I've tested and visualized Atlas Generation with and without the new flag - it works as expected. I've also ran the integration test with the new flag, it passed!

matthieun

The flow is much better and easy to read now. A few questions to be addressed! Thanks!!

matthieun · 2018-04-11T23:37:25Z

src/main/java/org/openstreetmap/atlas/generator/AtlasGenerator.java

+        return propertyMap;
+    }
+
+    private static Set<Shard> getAllShardsForCountry(final CountryBoundaryMap boundaries,


Isn't that going to be slow? It would probably benefit from using the grid index, like there:
https://github.com/osmlab/atlas-generator/blob/4.0.4/src/main/java/org/openstreetmap/atlas/generator/AtlasGenerator.java#L138-L204

Yes - that's a good point, let me update to use this!

matthieun · 2018-04-11T23:43:40Z

src/main/java/org/openstreetmap/atlas/generator/AtlasGenerator.java

+
+            // Slice the raw Atlas and filter any null atlases
+            final JavaPairRDD<String, Atlas> countrySlicedRawAtlasShardsRDD = countryRawAtlasShardsRDD
+                    .mapToPair(this.sliceRawAtlas(boundaries)).filter(tuple -> tuple._2() != null);


Here passing the boundaries means the driver will have to serialize it and pass it around to all the slaves. I guess the old flow does the same thing, not sure if there is a better way to do that... Would it be possible to only pass the boundary of the country that is being sliced? Not sure.

That's a good point, let me think of a better way to handle this or at least create an issue!

Created: #28 - let's come back to this. I think it's important, but something we can live with in the meantime.

matthieun · 2018-04-11T23:44:50Z

src/main/java/org/openstreetmap/atlas/generator/AtlasGenerator.java

+            countrySlicedRawAtlasShardsRDD.cache();
+            countrySlicedRawAtlasShardsRDD.saveAsHadoopFile(slicedRawAtlasPath, Text.class,
+                    Atlas.class, MultipleAtlasOutputFormat.class, new JobConf(configuration()));
+            logger.info("\n\n********** SAVED THE SLICED RAW ATLAS **********\n");


Here I believe you can now release the countryRawAtlasShardsRDD, because the next step, countrySlicedRawAtlasShardsRDD has already been cached.

matthieun · 2018-04-11T23:45:47Z

src/main/java/org/openstreetmap/atlas/generator/AtlasGenerator.java

+            countryAtlasShardsRDD.saveAsHadoopFile(
+                    getAlternateSubFolderOutput(output, ATLAS_FOLDER), Text.class, Atlas.class,
+                    MultipleAtlasOutputFormat.class, new JobConf(configuration()));
+            logger.info("\n\n********** SAVED THE FINAL ATLAS **********\n");


Same here, countrySlicedRawAtlasShardsRDD should be releasable here.

Knew I forgot something, good catch!!

matthieun · 2018-04-11T23:46:59Z

src/main/java/org/openstreetmap/atlas/generator/AtlasGenerator.java

+                    getAlternateSubFolderOutput(output, SHARD_STATISTICS_FOLDER), Text.class,
+                    AtlasStatistics.class, MultipleAtlasStatisticsOutputFormat.class,
+                    new JobConf(configuration()));
+            logger.info("\n\n********** SAVED THE SHARD STATISTICS **********\n");


And again, same here. Although at that point in the job, releasing RDD memory might not make much sense anymore.

Actually not here, as it is still used by the Deltas below!

matthieun · 2018-04-11T23:48:47Z

src/main/java/org/openstreetmap/atlas/generator/AtlasGenerator.java

+     * @return A Spark {@link PairFlatMapFunction} that takes a tuple of a country shard name and
+     *         atlas file and returns all the {@link AtlasDelta} for the country
+     */
+    private PairFlatMapFunction<Tuple2<String, Atlas>, String, AtlasDelta> computeAtlasDelta(


Make all those functions static so the whole SparkJob does not have to be serialized and passed around for the slaves to be able to call those methods!

And even better: push them to the helper!

Good call, let me do this!

matthieun · 2018-04-11T23:50:17Z

src/main/java/org/openstreetmap/atlas/generator/AtlasGeneratorHelper.java

+/**
+ * Utility class for {@link AtlasGenerator}.
+ *
+ * @author matthieun


Did I write that class??

You inspired it :)

…methods to helper class

matthieun

Perfect!

MikeGost added 2 commits April 6, 2018 15:12

Working end-to-end raw atlas flow

0602def

Merge branch 'dev' of https://github.com/osmlab/atlas-generator into dev

4244462

MikeGost added Enhancement New Feature labels Apr 6, 2018

MikeGost assigned matthieun Apr 6, 2018

matthieun requested changes Apr 6, 2018

View reviewed changes

MikeGost added 5 commits April 9, 2018 12:10

refactor to make Spark flow more apparent

78c155b

Merging changes from upstream

e44bcf8

Adding metrics and atlas deltas to new flow, moving calculations to h…

205d5c9

…elper functions

Upgrading to atlas 5.0.11

3bc990c

Updating integraton test, accounting for empty Clipped loading area

cd9db03

Implementing Atlas fetcher policy when sectioning atlases

551a282

Depending on atlas 5.0.12

8fdcd9f

MikeGost changed the title ~~Do Not Merge: Raw Atlas Flow Integration~~ Raw Atlas Flow Integration Apr 11, 2018

matthieun requested changes Apr 11, 2018

View reviewed changes

MikeGost added 2 commits April 12, 2018 10:47

Re-using shard calculation, unpersisting un-needed RDDs, moving more …

06bfd42

…methods to helper class

Removing google common Maps dependency

45fd460

matthieun approved these changes Apr 12, 2018

View reviewed changes

matthieun merged commit c5a9dd3 into osmlab:dev Apr 12, 2018

matthieun added this to the 4.0.5 milestone Aug 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Raw Atlas Flow Integration #25

Raw Atlas Flow Integration #25

MikeGost commented Apr 6, 2018 •

edited

Loading

matthieun left a comment

matthieun Apr 6, 2018

MikeGost Apr 7, 2018

matthieun Apr 6, 2018

matthieun Apr 6, 2018

matthieun Apr 6, 2018

MikeGost Apr 7, 2018

MikeGost Apr 7, 2018

MikeGost commented Apr 10, 2018

MikeGost commented Apr 10, 2018

matthieun left a comment

matthieun Apr 11, 2018

MikeGost Apr 11, 2018

matthieun Apr 11, 2018

MikeGost Apr 11, 2018

MikeGost Apr 12, 2018

matthieun Apr 11, 2018

matthieun Apr 11, 2018

MikeGost Apr 12, 2018

matthieun Apr 11, 2018

matthieun Apr 11, 2018

matthieun Apr 11, 2018

matthieun Apr 11, 2018

MikeGost Apr 11, 2018

matthieun Apr 11, 2018

MikeGost Apr 11, 2018

matthieun left a comment

Raw Atlas Flow Integration #25

Raw Atlas Flow Integration #25

Conversation

MikeGost commented Apr 6, 2018 • edited Loading

matthieun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

MikeGost commented Apr 10, 2018

MikeGost commented Apr 10, 2018

matthieun left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthieun left a comment

Choose a reason for hiding this comment

MikeGost commented Apr 6, 2018 •

edited

Loading