microg / IchnaeaNlpBackend

Backend for UnifiedNlp that uses Mozilla Location Service for geolocation.
116 stars 29 forks source link

Implements exponential backoff for Mozilla Location Services #39

Closed crankycoder closed 6 years ago

crankycoder commented 6 years ago

A patch to add exponential backoff for all 400 and 500 series errors from Mozilla Location Service.

My local builds seem broken so I'm just going to wait for Travis to do a build for me.

edit: My local build only seems to work with Android Studio

crankycoder commented 6 years ago

This PR should address the issues listed in #32 and which were addressed partially in 4c61799

crankycoder commented 6 years ago

cc/ @ckolos

ale5000-git commented 6 years ago

I don't know the code exactly but I think that in this way there will be cases where an app request the location in the middle of the request and it will get an updated lastRequestTime but an old lastResponse.

I think it would be better to leave it in the previous place but maybe add another variable to track when a request is in progress and do not allow another one at the same time (if this is the problem).

ale5000-git commented 6 years ago

I think there should be an upper limit to RATE_LIMIT_MS to avoid make it grown too much (or overflow) when there are server problems. And also a lower limit, I don't see it in the latest code change.

crankycoder commented 6 years ago

@ale5000-git this adds a floor value and a limit of 1024 times the floor value to set the ceiling backoff time.

I've also shuffled around a lot of the shared variables and protected them with synchronized methods and simplified the setup for a replayed location fix.

crankycoder commented 6 years ago

Can we get a review?

crankycoder commented 6 years ago

thanks @mar-v-in ! I think I've addressed all your review.

Regarding https://github.com/microg/IchnaeaNlpBackend/pull/39#pullrequestreview-128060110 - I've reworked the delay calculation for the back off timer to have a hard floor rate of 60sec.

crankycoder commented 6 years ago

You're not saving any memory materially by doing that. It's going to be a rounding errors compared to whatever the GC is doing to the heap.

Can we cut to the chase and get this merged in?

MicroG is currently consuming 10x the traffic of all other traffic combined on the MLS servers because of the query rate and the lack of backoff throttling is degrading the service for all users.

On Wed, Jun 13, 2018, 1:42 AM ale5000 notifications@github.com wrote:

@ale5000-git commented on this pull request.

In src/main/java/org/microg/nlp/backend/ichnaea/BackendService.java https://github.com/microg/IchnaeaNlpBackend/pull/39#discussion_r194999335 :

 private static final String PROVIDER = "ichnaea";
  • private long EXP_BACKOFF_RATE = 1;
  • private long EXP_BACKOFF_FACTOR = 0;

EXP_BACKOFF_FACTOR isn't a constant so as mar-v-in (lowercase starting camel case).

Just a personal note ( I always think about phones with low memory ;) ), does it really need to be long for just 1024 values? Similare note for RATE_LIMIT_MS_FLOOR and RATE_LIMIT_MS_PADDING.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/microg/IchnaeaNlpBackend/pull/39#pullrequestreview-128277631, or mute the thread https://github.com/notifications/unsubscribe-auth/AAAS3d04FB-ADVrr6FFmn0cKdpARkCNKks5t8NBegaJpZM4UVSCv .

ale5000-git commented 6 years ago

Only mar-v-in can merge it.