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

Added maxmind.default.override parameter to CRConfig to handle maxmind default locations #1866

Merged
merged 8 commits into from Feb 22, 2018

Conversation

rivasj
Copy link
Contributor

@rivasj rivasj commented Feb 8, 2018

Maintain a list of country code/coordinate pairs as: CountryCode;Lat,Long - in CRConfig.json to use instead of a default location for a given country. When a default location is indicated, TR checks if that country code has an entry specified in the config. If it does, it will set the client location to use the coordinates in that entry.

added documentation for geolocation.default.override profile parameter
@asfgit
Copy link
Contributor

asfgit commented Feb 8, 2018

Can one of the admins verify this patch?

@rivasj rivasj changed the title Added geolocation.default.override parameter to CRConfig to handle maxmind default locations Added maxmind.default.override parameter to CRConfig to handle maxmind default locations Feb 9, 2018
@rivasj rivasj force-pushed the 2.2.x_maxmind_default_override_3 branch from 97aef6f to ba0c0ca Compare February 9, 2018 20:27
@rivasj rivasj force-pushed the 2.2.x_maxmind_default_override_3 branch 2 times, most recently from 7ff4465 to 08cc6ba Compare February 12, 2018 22:23
@rivasj rivasj force-pushed the 2.2.x_maxmind_default_override_3 branch from 08cc6ba to 2b2a8df Compare February 12, 2018 22:33
@@ -158,6 +158,9 @@ Many of the settings for the different servers in a Traffic Control CDN are cont
+--------------------------+---------------+---------------------------------------------------------------------------------------------------------------------------------------+
| geolocation6.polling.url | CRConfig.json | The location to get the IPv6 GeoLiteCity database from. |
+--------------------------+---------------+---------------------------------------------------------------------------------------------------------------------------------------+
| maxmind.default.override | CRConfig.json | The destination geo coordinates to use for client location when maxmind returns a default location that matches the country code. |
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we enter more than 1 country at a time? i.e. US and Canada?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes, this parameter can be specified multiple times with different values. The documentation should be updated to reflect that.

@rivasj can you update the documentation to reflect this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've updated documentation to show support for multiple parameter values.

@@ -86,6 +87,8 @@
private final ConsistentHasher consistentHasher = new ConsistentHasher();
private SteeringRegistry steeringRegistry;

private final Map<String, Geolocation> defaultGeolocations = new HashMap<String, Geolocation>();
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpick, but variable might be more clear named defaultGeolocationsOverride. Skimming the code I got this confused with defaultLocation (which is what we call a geolocation from Maxmind w/o a city or postal code)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've renamed defaultGeolocations to defaultGeolocationsOverride

@elsloo elsloo merged commit 36eee10 into apache:master Feb 22, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants