Would Like To Contribute about riot-api-java HOT 13 CLOSED

Hey,

I prefer people to open issues rather than contact me on Twitter. It's sometimes hard to fit everything in 140 characters.

I'm down to add or merge stuff from your existing codebase, but it would have to take into consideration whats already in place, and likely be modified to fit. We don't want existing code breaking, or require people to completely rewrite their existing apps if they want to use the latest changes and features.

I don't feel like this should be a big issue with adding features like persistence and rate limit throttling, but when you talk about re-structuring things, we have to be careful about that.

We have a lot of contributors already, so I have no problem with you opening pull requests to contribute. I'm always happy to add more people as collaborators, but I want to make sure whoever I add as a collaborator understands the codebase enough to make quality contributions.

For example, I recently added Linnun the CEO of lolskill.net as a collaborator, because he has been a long time contributor, has numerous quality pull requests, has a large codebase using the library, and probably knows more about it than me at this point. :)

from riot-api-java.

Roughly how many projects are using your source? I'm working on uploading all the stuff I have done right to my own github.

from riot-api-java.

I'm personally a big fan of using design patterns, so I especially like your idea of making this library a factory. While this might have some advantages for the future, we obviously would break every single project using this code. It's hard to estimate how many projects are currently using this API and would be affected by this change, but with a proper note in the release notes I don't see much of a problem there.

Throttling and caching might be useful for some small project, but must definitely be optional features, since they won't work for big-scaling projects.
For example, I'm using one API code for a project that has multiple different applications, so I can't simply enter my rates into a single app. Also I run multiple instances of each app on different servers (for load balancing etc.) - making it even harder to properly throttle the rates.
Caching is a troubly topic for the same reasons - having multiple instances of an app across multiple servers makes it hard to cache data. You could be lucky to have the same request on the same server twice within the caching time, but in practice the second request is more likely to land on a different server - ultimatively you'll end up caching every request but will hardly ever make use of the cached data.
Also you may not want to cache every kind of method call. Let's say a user wants to look up his game, but he hits the button too early and gets a "not ingame" message. If he hits the button again 5 seconds later, the cache might keep telling him that he is not ingame, even though he actually is. Same issue the other way around: The game ended and the user started a new game, but the cache will still show his old game. These are just some of the issues I already had to deal with when building my custom caching methods - there is definitely alot to keep in mind.

While I don't mind having some basic caching or throttling methods, this definitely is not a high priority in my eyes.

from riot-api-java.

Yeah, it's really hard to tell how many projects are using this library, as I'm only aware of the people that tell me and can estimate based off the amount of questions and emails I get.

We don't want existing code breaking, or require people to completely rewrite their existing apps if they want to use the latest changes and features.

I noticed this quote by me might be misleading. It is preferred we don't break people's code, but we have done so in the past to up the quality of the library. When we do, we just mention it in the release notes (Like we did in the latest release). I didn't mean for it to sound like we can't improve the structure and quality of the existing code at all.

from riot-api-java.

All great answers! So I just pushed the stuff I have written up if you guys wanna take a look and see what's relevant to what you're doing. There are a lot of parallels.

from riot-api-java.

Thanks for providing your code!

Unfortunatelly I have alot of stuff to do this weekend, but I hope to be able to have a deeper look into your code in a few days. For now, I just took a quick look at your Throttle class because I was interested in how you implemented that. I'm very unhappy with how you hard-fix the rates in the code like this:

    if (isDevelopmentKey) {
        requestsPer10 = 10;
        requestsPer600 = 500;
    } else {
        requestsPer10 = 300;
        requestsPer600 = 180000;
    }

I got applications with rates higher than that, because those 180000 limits just aren't enough for some apps. In your API you assume everyone to have the exact same limits - which may be true for standard-sized apps, but just doesn't cover all apps.
Also why do you need a class for Throttling at all? Wouldn't it be enough to have a setRateLimits(int per10, int per600) method in your main class? (If not set, no throttling will be done. If set, throttling will be done according to the given rates).

On a small side note, I like your use of a StringBuilder to build URLs within the API implementations. Looking forward to find some more goodies in your code in a few days.

from riot-api-java.

Thanks for providing your source. I should have time this weekend, so I'll look over it too.

from riot-api-java.

I'm very unhappy with how you hard-fix the rates in the code like this:

Yeah, me too.

I would actually like to solve the general problem of: given N time windows of M requests each design a class that maximizes number of requests from X clients without any client ever receiving a 429 response. The current RiotApiThrottle does maximize number of requests without getting a single 429, however X is bound to 1 (that is only a single client can be in use at once, Linnun raised this point earlier when he mentioned load balancing) and the time windows are hardcoded. There is a technology called HornetQ that can help make it possible for distributed computing nodes (like load balanced servers) to synchronize critical components like rate throttles. Setting up HornetQ can be nasty, nor do I know how invasive it would be to make existing code compatible.

Additionally, a single Thread object pulls tasks from a queue. In a real high-speed environment this queue would become prohibitively large and, because it's managing an ArrayBlockingQueue, will actually cause the thread posting tasks (see throttle.post(..)) to block. In a real world application this would cause your UI thread or request processing thread to hang, not good.

Finally, it should be using a ThreadPool not a single thread. I did try using a thread pool originally, but I ran into some bugs on Riot's side. The best thing the current throttle did for me was show me the non-obvious issue that you have to time requests from the time you receive the response, not the time you send the response. It seems that Riot sets times on when the request completes. They also seem to like a 1.5 second buffer. So the throttling service is certainly a work in progress.

from riot-api-java.

What about using guava - RateLimiter for Rate Limiting?
Add the rate limits to e.g. riot-api.properties. When nothing is defined, ignore the rate limiting, else use those from the file.

from riot-api-java.

Well I ended up writing my own Riot-API, but here is how I worked out the Rate Limiting:

I am using this Library:

<dependency>
<groupId>org.isomorphism</groupId>
<artifactId>token-bucket</artifactId>
<version>1.6</version>
</dependency>

List<TokenBucket> tokenBuckets = new ArrayList<>();
tokenBuckets.add(TokenBuckets.builder().withCapacity(10).withYieldingSleepStrategy().withFixedIntervalRefillStrategy(10, 10, TimeUnit.SECONDS).build());
tokenBuckets.add(TokenBuckets.builder().withCapacity(500).withYieldingSleepStrategy().withFixedIntervalRefillStrategy(500, 10, TimeUnit.MINUTES).build());
[...]
tokenBuckets.parallelStream().forEach(tokenBucket -> { tokenBucket.consume(); });
[...]
if (responseCode == 429) { if (connection.getHeaderField("Retry-After") != null) Thread.sleep(1000 * .parseInt(connection.getHeaderField("Retry-After"))); }

from riot-api-java.

The token bucket approach is really close to what I want. I still get a few 429's when I run it, but it's pretty elegant. Thanks for turning me on to that library. I'm currently working this issue with a thread pool and I'm really close to having a solid, truly asynchronous, blazing fast, memory efficient method of rate controlling. I am also working on leaving hooks in place to integrate load balancing and the likes.

But I really like this simplicity of this approach. 10 / 10.

from riot-api-java.

In case you are succesfull, I'd be happy if you share you'r solution.

Try this: https://gist.github.com/cocojack42/8e610120d93673d63ade
What do you think? It has no multi-node support, but I assume this would be very hard to implement without a db. The consuming of the tokens is syncronised, but the connections are multithreaded.

from riot-api-java.

So the main difference with our stuff is that the solution that I came up with is more of a scheduler than a throttler. That is, when a request comes in the scheduler looks into the future and determines when a request can execute. It does not delay until a time has passed then allows a request to execute. This allows for some slick usage of Java's scheduling thread pool. My repo is https://github.com/dancarpenter21/lol-api-java and if you want to check stuff out look at the class org.dc.riotapi.service.scheduler.RiotApiScheduler.java. It's a pretty complex class but the gist is that the schedule method adds a callback to a ThreadPool for execution. The service then analyzes network rates and conditions to approach an ideal speed based on rate rules and outside factors. The issue that's going to arise now is all these asynchronous calls are resulting in callback hell when I want to do anything real. Since every request that I want to be throttled has to be passed in as a Runnable essentially there's a lot of inner class nesting going on. I also need to add a nice way to handle 429 codes but I do have a strategy in mind. I might be able to add that tonight. In all I think my approach will perform well, but might be too unwieldy to be useful. I definitely feel that a Reactive approach is the way to go here.

from riot-api-java.