-
Notifications
You must be signed in to change notification settings - Fork 0
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
Any interest / benefit in reducing number of GraphQL API calls required by combining similar queries? #64
Comments
Well yes that is a possibility. I made all methods separate to be able to use what everybody exactly want without collecting lots of data that is not been used. So for example combining all methods would be an impossible task as the query and the method would become so large. Combining smaller methods with a small query would be a option as long as they somehow belong together So @mosource21 would you do some more proposal's? Maybe mainRating, Metacritic and Votes could be combined? they all belong to "rating stuff" |
Yes completely agree it is a balancing act. I wouldn't say it is slow at the moment it just could be much more efficient if we think there it is going to be a benefit. Everyone is using the library different but I could be using 10 API calls per title to only get back a relative small amount of data.
Yes that sounds ideal. All assuming GraphQL can get all three items together?
These are specific to my use and exposure of all the library functions is limited so I may not have a great overview. Not sure if they will meet any criteria you are setting for "belonging together". If the method is returning "complex / multi fielded" data I thought it best to just leave as it is but anything which is just a single value or a list of single values I thought was fair game. Proposal 2 Proposal 3 Try It First Before Investing To Much As before more than happy to be involved in the process if you want input, code suggestions and help with testing etc. |
Thanks for your input, i'l take all this into consideration. I'll check first if those three even can be combined at all @GeorgeFive what is your view on this? |
I don't think it's a bad idea. I'd like to keep compatibility of course, and the one thing I do enjoy now is how easy it is to find something. Could that be kept when combining? For example, if I want to get languages... I really don't even need the wiki or docs or anything, I can just look at $langs. Plot? It's over there in... dun dun dun... $plot. If we start combining stuff, ie, $grading contains rating, metacritic, votes, etc... that may make it more difficult to find stuff. I also happen to enjoy how everything is spread out, because I can get exactly what I need and nothing else. For example, I actually do get keywords, but not language and genres. I don't think this is a HUGE issue, but it's just something to keep in mind. |
Mm still thinking about this Combining wouldn't much help i guess, let me give a example for my use case |
Thanks @GeorgeFive I also enjoy that everything is split out, so everybody can use exactly what they need. And combining can make that method slower as there is more data to be processed. So it may not be the best solution after all i guess. I do appreciate you all for thinking about how to make it better though, so thank you all! |
No problem that is why I asked. I can't argue it does work great. I may have a try myself though is there any resources on the basics of GraphQL and specifics for IMDB (fields name list) that you use? Can I just change the query name to whatever I like and add more fields like below and it should just work or does things like query name (NewRating in my example below) need to be something specific to IMDB to work etc?
|
Documentation is here QueryName is indeed anything you like as long as you use the same name in api request ("TitltleYear" must be the same as queryName) You can use multiple query's like in your example. I really doubt combining would benefit speed but it could make some difference i suppose If you really want to know everything about how GraphQL works you have to study al lot though, it is a lot! |
@mosource21 @GeorgeFive I can for example add titleYear() to __construct() so that the basic info is always available? Example We can add other basic info that anyone would want to titleYear() accessible through separate methods So combining stuff might not a bad idea after all? Basis info could/would be: |
Not a bad idea... it won't do much for me (this would save me two calls total), but if it helps others, I'm definitely for it. |
In my case this will save a total of 9 calls as i use all. I'll leave this on the back burner for now |
My original aim with this question was to try to reduce the "load" on the IMDB servers and to some degree speed up things at my end as each request has an overhead for me. If IMDB use the GraphQL interface themselves on the website I guess it may not be a huge priority as this library usage would just be a drop in the ocean but I would be very surprised there isn't a limit on the number of requests before IMDB notice and temporarily block access.
Strictly speaking you should always have caching enabled. I do not see any reason why you need "live" information from IMDB. If you have the cache enabled it doesn't really matter calling titleYear() three times as the second and third requests should use the cache. Even if you cache requests for a day (or even a few hours if you do have a need for more "live" information) it will have a decent impact of the "load" on the IMDB servers and therefore reduce the possibility of them noticing or blocking accessing. It also has the benefit of speeding up the library as any repeat requests will be served from the cache.
The main problem is everyone will have different needs. I have combined some of the more basic requests and achieved a significant improvement for my specific usage - this is not guaranteed and will all depend on how you are using the library. It will make updating the library a bit more difficult but not much so as my changes are quite modular. I think an argument can be made for combining similar things (see original proposals/suggestions) but take it too far and combine everything you are sending a massive GraphQL query which may not be the best thing to do.
See above - for me I would just end up having to comment out this line in __construct as I have my own implementation for the data normally retrieved by titleYear(). |
This is a fair point of course! And yes we all have different use cases and different needs. So i think that our main conclusion is that this is, for the main use of this library, not the way to go. For now this will be on the backburner but i'll keep it in mind. Thanks everyone for your input! |
I'm thinking again about this and came up with this idea Would it be of any interest to make a second Title class (TitleCombined for example) and use that to make combined methods? Still it would be not suitable for everyone but it might be a start? |
I don't think it's a bad idea, I don't see a problem there. Could be interesting! |
Okay i'm going to create it. I need some input as to what methods need/can be combined |
I did made a new class TitleCombined, it contains (for now) one main method that fetches the main info of a movie or series. It contains data what is visible on IMDb movie pages inside the black background part at the top (minus the mpaa)
|
I think this idea is near perfect. The functionality is focused (i.e. you are aiming for black background part at the top) and as everyone has different needs it can then be used as an example / template for customizing. The only slight disadvantage is if you need to fix / update / enhance Title.php you also need to update TitleCombined.php so there is an admin overhead. A couple of minor suggestions (untested): 1. Update TitleYear in both these lines to TitleCombined
2. Include the canonicalId so you can check that the imdbid hasn't been updated if you want
|
Thanks for your comments! But i don't understand what you mean at point 1? Point 2: |
Nothing of major importance purely a code readability suggestion. Change the name of the GraphQL query from TitleYear to something more descriptive - TitleCombined (could equally be just Combined, BlackBox etc but TitleCombined seems the best fit in my mind 😃)
|
Ah i understand what you mean now, i will change it to something more meaningful, thanks. |
@mosource21 |
The library (title.php) currently uses many calls to the IMDB API.
All these separate GraphQL API calls are relatively slow and "costly" on resources.
Is there any interest / benefit on perhaps trying to reduce the number of API calls?
Proposal 1
Is is possible to combine the GraphQL for the "mainRating" and "Metacritic" into a single GraphQL query and get both values with a single API call?
Thanks
The text was updated successfully, but these errors were encountered: