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

Switch to GeoIP2, add db update and locale options. #1748

Merged
merged 8 commits into from
Jan 20, 2018

Conversation

k-jiang
Copy link
Contributor

@k-jiang k-jiang commented Jan 5, 2018

Fix #1745 #1746

Since MaxMind is going to drop support on Legacy GeoIP db files, I decided to upgrade EssentialsGeoIP to work with the newer and more featured GeoIP2 API.

The change of this PR include:

  1. switch to GeoIP2 API
  2. add option to update GeoIP2 db files
  3. add option to show geolocation with locale

Some minor changes include:

  1. update the locale files to reflect the correct size of new mmdb files.

Changes that I have thought of but did not included:

  1. Custom message when query with GeoIP2 API generates exceptions.
    GeoIP2 API now support several exception messages, including the "must have" AddressNotFoundException (see EssentialsGeoIPPlayerListener.jar line 97 to 106). It should be a good idea to add some custom messages when such a exception is caught. Yet I'm not sure if it is a good idea to do so since I could only handle English and Chinese (CN, HK and TW).

If you think there are option(s) I have added are unnecessary, please contact me and I will remove them removed for you.

add option to update GeoIP2 db files
add option to show geolocation with locale
@k-jiang k-jiang mentioned this pull request Jan 5, 2018
@drtshock
Copy link
Member

drtshock commented Jan 5, 2018

This brings EssentialsXGeoIP.jar up from 117.31kb to 4.3mb. Can we shade less of apache commons, or is there an alternative solution to untarring files?

downgrade jackson-databind to 2.7.9.2 just to lose 200k more.
@k-jiang
Copy link
Contributor Author

k-jiang commented Jan 5, 2018

Well 1.6mb is the best I could do so far.
Now the remaining problem is, the official GeoIP2 API uses a dependency called jackson-databind which is bloated. This dependency itself occupies 1.2-1.3mb in size. I even tried to downgrade it as well just to gain some extra 200k advantage. Notice that v2.7.x is the lowest version it could run.

I think the other way out is to rewrite the GeoIP2 API by hand, but it is definitely going to be as pain as hell.

Let me know if you have any more idea.

@@ -26,14 +26,57 @@

<dependencies>
<dependency>
<groupId>net.ess3</groupId>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You removed essentials from the dependencies. Breaks this.

Copy link
Contributor Author

@k-jiang k-jiang Jan 5, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some how, with this dependency, maven includes the whole ess into GeoIP.jar. To be honest I'm not sure how to do it properly. At least it doesn't cause problem when I tested it :P

@k-jiang
Copy link
Contributor Author

k-jiang commented Jan 5, 2018

Ok I think I have a proper solution now. Trying to use maven-shade-plugin, which should get the job done.
But I need more time on testing, may push a new commit by tomorrow.

@drtshock
Copy link
Member

drtshock commented Jan 5, 2018

Sweet. Thank you for your work on this ❤️

@k-jiang
Copy link
Contributor Author

k-jiang commented Jan 6, 2018

I think that's it. 1.1Mb is the best I could make. (F the jackson lib).

P.S. There is an interesting maven plugin called com.github.wvengen:proguard-maven-plugin which claimed to be able to shrink jar. Have tried and shrink the jar to ~700k but it generate exception during loading. Don't know exactly how to use it. But so far I think 1.1Mb is good enough. (F the jackson again).

@drtshock
Copy link
Member

drtshock commented Jan 6, 2018

Nice. I'll take a look at this soon. Lets see if we can get @SupaHam to approve :)

@mdcfe mdcfe added the bug: confirmed Confirmed bugs in EssentialsX. label Jan 6, 2018
@SupaHam
Copy link
Member

SupaHam commented Jan 16, 2018

Please remove javatar dependency and autotest.sh.

I'll need to test this later to make sure it works fine.

@k-jiang
Copy link
Contributor Author

k-jiang commented Jan 17, 2018

@SupaHam javatar is required to unpack maxmind's mmdb2 file from .tar.gz. You can check here for their new packing format.

I don't know why you want to remove it. Maybe you want me to use some other alternative instead?

@SupaHam SupaHam merged commit 938f94e into EssentialsX:2.x Jan 20, 2018
@SupaHam
Copy link
Member

SupaHam commented Jan 20, 2018

Thank you very much for reaching out to us before the breakages happened.

The reason I wanted javatar removed is because of jar footprint, we need to make sure we're not adding things needlessly without considering better alternatives.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug: confirmed Confirmed bugs in EssentialsX.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants