Closes #2736 Import Geonames Data#2782
Conversation
diasf
left a comment
diasf
left a comment
There was a problem hiding this comment.
Please add some tests. It's a good candidate for an integration test importing a sample file and querying the import.
At this stage GEONAME fields in datasets are indexed with only the ID of the geoname. So datasets won't be findable using any of the geoname text fields. Is this the desired behavior?
| // country codes repeat often so deduplicating them saves a lot of memory | ||
| private final Map<String, String> countryCodes = new HashMap<>(); | ||
| // feature codes repeat often so deduplicating them saves a lot of memory | ||
| private final Map<String, String> featureCodes = new HashMap<>(); |
There was a problem hiding this comment.
I don't think you will be able to import the whole file at once on most of our environments. Most have < 3GB of head room. So you would need to split the file. And you would also have to make sure you don't split it in the middle of a country, otherwise you will have wrong results.
As with the optimizations, I'm just concerned about readability. I mean, if they were obvious you wouldn't need extra commentary around them.
That being said, if you think the current approach is good enough, then let's leave it at that.
There was a problem hiding this comment.
File isn't in this repo anynmore. Please sync with the main develop branch.
| public class GeoNamesIT extends WebappArquillianDeployment { | ||
|
|
||
| private GeoNameIndexingService indexer; | ||
| private GeoNameDataFinder finder; |
There was a problem hiding this comment.
Why aren't those injected as well?
There was a problem hiding this comment.
so that we can initialize them with
final SolrClient solr = new TestSolrClientFactory().produceGeoNameSolrClient();
There was a problem hiding this comment.
Injection of @GeoNameSolrClient should also work in IT tests.
There was a problem hiding this comment.
This should still be addressed, testing the correct wiring is part of IT tests.
| private GeoNameDataFinder finder; | ||
| @Inject | ||
| @GeoNameSolrClient | ||
| private SolrClient solr; |
|
|
||
| final SolrClient solr = Mockito.mock(SolrClient.class); | ||
| when(solr.addBeans(anyCollection())).thenThrow(RuntimeException.class); | ||
| this.indexer = new GeoNameIndexingService(solr); |
There was a problem hiding this comment.
This is more of a unit test for GeoNameIndexingService.
| this.indexer.importNames(in); | ||
| fail("Exception expected"); | ||
| } catch (Exception e) { | ||
| // pass |
There was a problem hiding this comment.
There's assertThatThrownBy which can be used in this case.
lbownik
force-pushed
the
2736_Import_Geonames_Data_3
branch
2 times, most recently
from
119e0b3 to
ab1dec2
Compare
lbownik
force-pushed
the
2736_Import_Geonames_Data_3
branch
from
ab1dec2 to
e7daa25
Compare
3 participants
No description provided.