Added CountryToShardList Caching #202

jwpgage · 2018-08-22T23:44:59Z

Description:

This PR adds command CountryToShardListCacher (name open for suggestion), which is a command very similar to CountryToShardListing but instead of writing a list of CountryShards, writes to file a country followed by all shards that intersect that country (each country and shard list pair is one line). The PR also adds CountryToShardListCache, which reads the file produced by CountryToShardListCacher, and stores these country/shardList pairs in a HashMap.

UPDATE: After PR comments and offline discussion, this has been refactored to a single class CountryToShardListCache, which can be constructed from scratch using a sharding, country list, and boundary file, or can be constructed from file. The class offers a save(WritableResource output) function, which writes to file the MultiMap of country to intersecting shards. The class also offers a getShardsForCountry(String country), which returns an Optional<List<SlippyTile>> from the MultiMap.

Potential Impact:

This does not change any existing libraries, but is instead a new feature.

Unit Test Approach:

~~The unit test added tests creating a CountryToShardListCache object from a .txt file generated by CountryToShardListCacher, and then tests getting the shards that intersect DMA.~~

The unit tests test getting the shard list for a specific country, creating an instance from scratch, creating an instance from file, and ensures that these have the same output.

Test Results:

N/A

In doubt: Contributing Guidelines

MikeGost

Clean and easy to understand, thanks @jwpgage! Couple of minor nits along the way.

MikeGost · 2018-08-23T17:20:00Z

src/main/java/org/openstreetmap/atlas/geography/boundary/CountryToShardListCache.java

+public final class CountryToShardListCache
+{
+    private static final Logger logger = LoggerFactory.getLogger(CountryToShardListCache.class);
+    private final HashMap<String, List<SlippyTile>> countryToShards = new HashMap<>();


Nit: Consider using MultiMap here instead of HashMap.

MikeGost · 2018-08-23T17:21:30Z

src/main/java/org/openstreetmap/atlas/geography/boundary/CountryToShardListCache.java

+                final String shardList = countryAndShardList[1];
+                final List<SlippyTile> shards = new ArrayList<>(
+                        Arrays.asList(shardList.split("\\s*,\\s*"))).stream()
+                                .map(string -> SlippyTile.forName(string))


Nit: SlippyTile::forName

MikeGost · 2018-08-23T17:24:22Z

src/main/java/org/openstreetmap/atlas/geography/boundary/CountryToShardListCacher.java

+                    {
+                        final List<String> shardNames = shardSet.stream()
+                                .map(shard -> shard.getName()).collect(Collectors.toList());
+                        writer.writeLine(String.format("%s||%s", country, shardNames)


Do you want to pull out the || as a delimiter constant?

MikeGost · 2018-08-23T17:25:07Z

src/main/java/org/openstreetmap/atlas/geography/boundary/CountryToShardListCache.java

+        {
+            file.lines().forEach(line ->
+            {
+                final String[] countryAndShardList = line.split(Pattern.quote("||"));


Nit: If you pull out the delimiter constant in the Cacher, than you reference it here, so if it ever changes, this code won't break.

MikeGost · 2018-08-23T17:25:35Z

src/main/java/org/openstreetmap/atlas/geography/boundary/CountryToShardListCacher.java

+    private static final Switch<CountryBoundaryMap> BOUNDARIES = new Switch<>("boundaries",
+            "The country boundaries.", value ->
+            {
+                final Time start = Time.now();


Lot going in when parsing a switch, recommend pushing this to a private method.

MikeGost · 2018-08-23T17:27:55Z

src/test/java/org/openstreetmap/atlas/geography/boundary/CountryToShardListCacheTest.java

+public class CountryToShardListCacheTest
+{
+    @Test
+    public void testGetShardNamesForCountry()


Maybe add a quick test for a non-existent country to verify nothing breaks?

AlexHsieh22 · 2018-08-23T17:44:00Z

src/main/java/org/openstreetmap/atlas/geography/boundary/CountryToShardListCache.java

+    private static final Logger logger = LoggerFactory.getLogger(CountryToShardListCache.class);
+    private final HashMap<String, List<SlippyTile>> countryToShards = new HashMap<>();
+
+    public static CountryToShardListCache create(final Resource file)


Throwing a random idea out here - would it be useful to have the CountryToShardListCache also take in a CountryBoundaryMap and Sharding? Then CountryToShardListCache can support read() and write() methods and can essentially become a resource. Not sure what the scope of this very useful functionality is though!

Another advantage is that it would also make the CountryToShardListCacher really only a thin layer. The logic in the CountryToShardListCacher can then be in one place and users won't have to duplicate that code to write CountryToShardList files.

Great idea, working on this now

matthieun

Nice @jwpgage! A few suggestions. It would also be nice to have extra javadoc comments on the methods that are public facing.

matthieun · 2018-08-23T23:42:56Z

src/main/java/org/openstreetmap/atlas/geography/boundary/CountryToShardListCache.java

+                });
+    }
+
+    public CountryToShardListCache(final Resource file)


Can the resource be called resource?

matthieun · 2018-08-23T23:43:54Z

src/main/java/org/openstreetmap/atlas/geography/boundary/CountryToShardListCache.java

+        }
+    }
+
+    public Optional<List<SlippyTile>> getShardsForCountry(final String country)


Here I think returning an empty list is simpler than having an optional. Thoughts?

Definitely simpler, I changed it

matthieun · 2018-08-23T23:47:14Z

src/main/java/org/openstreetmap/atlas/geography/boundary/CountryToShardListCache.java

+    private static final String DELIMITER = "||";
+    private final MultiMap<String, SlippyTile> countryToShards = new MultiMap<>();
+
+    public CountryToShardListCache(final CountryBoundaryMap boundaries, final StringList countries,


How about adding another constructor that uses all the countries in the boundarymap and dos not require a list of countries?

matthieun · 2018-08-23T23:49:33Z

src/main/java/org/openstreetmap/atlas/geography/boundary/CountryToShardListCache.java

+ *
+ * @author james-gage
+ */
+public final class CountryToShardListCache


How about making this a Command that takes a path to a country boundary map file, a sharding tree file, and an optional list of countries, and saves it to a result file?

MikeGost

Nice work @jwpgage - very clean final product. Despite the intuitive naming, still requesting a +1 to Matthieu's suggestion of adding some java doc to public facing methods.

MikeGost

Great stuff!

matthieun

Thanks @jwpgage, sorry I had missed those to logger error statements. I think they should be exceptions instead. Otherwise everything looks great! Once that changes it is ready for merge!

matthieun · 2018-08-24T17:38:52Z

src/main/java/org/openstreetmap/atlas/geography/boundary/CountryToShardListCache.java

+        }
+        catch (final Exception e)
+        {
+            logger.error("Error while reading CountryToShardListCache file!", e);


Throw a CoreException here that references the other one! I don't think we want a partially built file, with an error message buried in the logs..

matthieun · 2018-08-24T17:39:05Z

src/main/java/org/openstreetmap/atlas/geography/boundary/CountryToShardListCache.java

+        }
+        catch (final Exception e)
+        {
+            logger.error("Error while writing CountryToShardListCache to file!", e);


jwpgage added 2 commits August 22, 2018 16:21

Added CountryToShardListCache + test

7f1bbd4

general cleanup

129c550

jwpgage added the New Feature label Aug 22, 2018

jwpgage requested review from matthieun and MikeGost August 22, 2018 23:45

cleanup

d13fc62

MikeGost requested changes Aug 23, 2018

View reviewed changes

AlexHsieh22 reviewed Aug 23, 2018

View reviewed changes

jwpgage added 2 commits August 23, 2018 15:34

Streamlined to one class, added more tests

0ff3383

Removed unused country shard lists from unit test resource

c2502bb

matthieun requested changes Aug 23, 2018

View reviewed changes

Made CountryToShardListCache a Command

b48a80d

MikeGost reviewed Aug 24, 2018

View reviewed changes

Added javadoc comments on public facing methods

a15333a

MikeGost previously approved these changes Aug 24, 2018

View reviewed changes

matthieun requested changes Aug 24, 2018

View reviewed changes

changed how exceptions are handled

36b36c6

jwpgage dismissed MikeGost’s stale review via 36b36c6 August 24, 2018 17:45

matthieun approved these changes Aug 24, 2018

View reviewed changes

matthieun merged commit 85caca6 into osmlab:dev Aug 24, 2018

jwpgage deleted the shardlistcache branch August 24, 2018 21:48

matthieun added this to the 5.1.9 milestone Aug 27, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Added CountryToShardList Caching #202

Added CountryToShardList Caching #202

jwpgage commented Aug 22, 2018 •

edited

Loading

MikeGost left a comment

MikeGost Aug 23, 2018

MikeGost Aug 23, 2018

MikeGost Aug 23, 2018

MikeGost Aug 23, 2018

MikeGost Aug 23, 2018

MikeGost Aug 23, 2018

AlexHsieh22 Aug 23, 2018

jwpgage Aug 23, 2018

matthieun left a comment

matthieun Aug 23, 2018

matthieun Aug 23, 2018

jwpgage Aug 24, 2018

matthieun Aug 23, 2018

matthieun Aug 23, 2018

MikeGost left a comment

MikeGost left a comment

matthieun left a comment

matthieun Aug 24, 2018

matthieun Aug 24, 2018

Added CountryToShardList Caching #202

Added CountryToShardList Caching #202

Conversation

jwpgage commented Aug 22, 2018 • edited Loading

Description:

Potential Impact:

Unit Test Approach:

Test Results:

MikeGost 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

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

MikeGost left a comment

Choose a reason for hiding this comment

MikeGost left a comment

Choose a reason for hiding this comment

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

jwpgage commented Aug 22, 2018 •

edited

Loading