Skip to content
Snippets Groups Projects

Added CSV support for maxmind subnet data

Merged Martin Lowe requested to merge github/fork/autumnfound/malowe/master/2 into master

Added CSV support through OpenCSV lib for CSV parsing. Moved logic for retrieving data to Service. Updated documentation to reflect some of the changes to the application. Took first pass at native image support.

Signed-off-by: Martin Lowe martin.lowe@eclipse-foundation.org

Merge request reports

Merged by Christopher GuindonChristopher Guindon 5 years ago (Nov 18, 2019 4:43pm UTC)

Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • Author Maintainer

    This PR currently does not compile in native image mode. I'm looking for a little help in completing this PR to ensure that we can compile and push code to production as soon as possible.

    The issues seem to stem from stripped class definitions for Apache Logging (used by OpenCSV), and City/Country responses (from Maxmind). I've attempted to add a reflection configuration file for 3rd party sources (https://quarkus.io/guides/writing-native-applications-tips#registering-for-reflection), but it can't be seen at package time for some reason even though I followed the instructions to the letter. I've looked for fixes for this online, but it seems like this should work otherwise.

    @mbarbero you have some experience with Quarkus, have you had to register reflection sources in this way before? Do you have any insight?

  • Created by: mbarbero

    I never did that for a Quarkus app, only for standalone app, and that's how it's done for any app targeting SubstrateVM.

    As your external dependencies (Maxmind and Apache Logging) are not Quarkus Extensions, the Quarkus build infra does not have any magic to generate the reflection sources for you.

  • Created by mbarbero

    Review: Approved

    Could you please test how low you can set the heap and still be able to start the app without getting an OOM? This will give us a rough idea of the required memory to keep the subnet in memory.

    Start small: -Xmx32m, Xmx64m, Xmx128m...

  • What I don't understand while building a native app is that with this application I get the following error:

    Error: Invalid Path entry /git/geoip-rest-api/reflection-config.json
    Caused by: java.nio.file.NoSuchFileException: /git/geoip-rest-api/reflection-config.json

    I tried building a native app using the source code from https://github.com/quarkusio/code.quarkus.io and I am getting the same error:

    Error: Invalid Path entry /git/code.quarkus.io/reflection-config.json
    Caused by: java.nio.file.NoSuchFileException: /git/code.quarkus.io/reflection-config.json

    I did notice that they have a separate docker file where they copy these files:

    https://github.com/quarkusio/code.quarkus.io/blob/master/src/main/docker/Dockerfile.native.multistage

  • Created by: mbarbero

    I'll have a look.

    Honestly, I would not worry about compiling the native app... IMO, this is not the mode we would deploy in production for now. This is still very young and can "unknown" in terms of tools to profile / debug once debug. Also, plain old Java is just fast enough: we don't care about the startup time being 3sec or 10ms, we won't start this app thousands of time a day to make it important. Plain old Java also has better throughput which is better for long lived apps like this one.

  • Christopher Guindon
  • Christopher Guindon
  • Christopher Guindon
  • Author Maintainer

    I managed to get it down to 256mb, any lower and quarkus fails out OOM. Looks like open csv is kinda memory hungry. Do we want to look at optimizing this?

  • Please register or sign in to reply
    Loading