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

Feature request: change clampSimultanousFetchSize() #1468

Open
3 tasks
gamebeaker opened this issue Sep 7, 2024 · 9 comments
Open
3 tasks

Feature request: change clampSimultanousFetchSize() #1468

gamebeaker opened this issue Sep 7, 2024 · 9 comments

Comments

@gamebeaker
Copy link
Collaborator

Change clampSimultanousFetchSize() so that it is the same as this.minimumThrottle = 3000;
I think a variable is more elegant than a function and at the moment the user is unable to overwrite clampSimultanousFetchSize().
Todo:

  • Change clampSimultanousFetchSize() to a variable
  • If "Override Minimum Throttle Value (where applicable)" is checked allow overwrite
  • Rename "Override Minimum Throttle Value (where applicable)"
@dteviot
Copy link
Owner

dteviot commented Sep 8, 2024

@gamebeaker

User is not supposed to be able to override clampSimultanousFetchSize(). It's applied on a per parser (i.e. Site) basis, for sites that don't like rates of more than 1 page at a time.

@gamebeaker
Copy link
Collaborator Author

gamebeaker commented Sep 9, 2024

@dteviot i would argue that this.minimumThrottle is the same as in that if it is defined in a parser it is normally required or you get rate limited. So the user should not change minimumThrottle but are still able todo it if they want.
I would be ok with either (the user is able/ unable to overwrite both values). I just want it to be consistent as both option influence fetches/ second.

@dteviot
Copy link
Owner

dteviot commented Sep 9, 2024

@gamebeaker
I agree the rate limiting is something a mess.
Perhaps we should take a step back, decide what the requirements/goals need to be, and proceed from there.

@dteviot
Copy link
Owner

dteviot commented Sep 9, 2024

  1. User should be able to set rate on a per-site/parser basis.
  2. Default rate is 1 page at a time, 3 seconds per fetch.
  3. Should be as easy for users to use as possible.
  4. Some sites mandate 1 page at a time. (e.g. Royal Road's TOS.) For these sites, users can not override pages at a time.

@gamebeaker
Copy link
Collaborator Author

gamebeaker commented Sep 9, 2024

@dteviot and we don't violate the TOS...
image
but i get it WebToEpub should not increase the server load substantial like a ddos.

@gamebeaker
Copy link
Collaborator Author

  1. User should be able to set rate on a per-site/parser basis.

Isn't that the case currently? Or do you mean the settings should be saved per site/ parser?

  1. Default rate is 1 page at a time, 3 seconds per fetch.

Change in html default value should do the trick.

  1. Should be as easy for users to use as possible.
  2. Some sites mandate 1 page at a time. (e.g. Royal Road's TOS.) For these sites, users can not override pages at a time.

I would interpret it like this:

class Parser {    
    constructor(imageCollector) {
        this.minimumThrottle = 0;
        this.maxPagesToFetchSimultaneously = 8;

and a function similar to getRateLimit() for getFetchSimultaneously() where this.maxPagesToFetchSimultaneously is upper limit instead of lower limit and that the user is unable to overwrite this.maxPagesToFetchSimultaneously to a higher number.
For consistency sake i would delete the option checkbox "Override Minimum Throttle Value".

@dteviot
Copy link
Owner

dteviot commented Sep 10, 2024

Isn't that the case currently? Or do you mean the settings should be saved per site/ parser?

I thought if user makes changes in the UI, they are applied to every site/parser. Or did your changes alter that?
And yes, I also mean settings should be set and saved per parser. So, if someone finds site X will allow faster downloads, then they WebToEpub will use that faster speed, without changing for other sites.

@gamebeaker
Copy link
Collaborator Author

Ok i am not sure how this should be implemented.

Or did your changes alter that?

I haven't made changes at the moment just ideas.

I thought if user makes changes in the UI, they are applied to every site/parser.

And yes, I also mean settings should be set and saved per parser. So, if someone finds site X will allow faster downloads, then they WebToEpub will use that faster speed, without changing for other sites.

Aren't these two things contradictory?
Example:
I am using random sites with random values.
a.com is able to handle 8 FetchSimultaneously with 0 sec/ wait. The user changes the options accordingly. -> Now all sites have these new option if they aren't constrained in the parser through clampSimultanousFetchSize() and minimumThrottle (your first scenario). But if the user now changes these options for b.com to 4 FetchSimultaneously and 3 sec/ wait it also changes for a.com to the same options i am not sure how the changes should be saveable per parser but still apply to every parser. (A button that says save download speed for website?)

As an other idea:

class Parser {    
    constructor(imageCollector) {,
        this.minimumThrottle = 0;
        this.defaultThrottle = 3;
        this.maximumPagesToFetchSimultaneously = 8;
        this.defaultPagesToFetchSimultaneously = 1;

In the html droppdown to select add a new option named "default" as the new default value.
If this value is selected choose the default value from the parser, which could be overwritten in the specific parser. What is the difference between minimumThrottle and defaultThrottle? (it could be only TOS as the default value should protect before HTTP 429) Same question for maximumPagesToFetchSimultaneously and defaultPagesToFetchSimultaneously.
As soon as the user changes the value manually this change is only saved for this one parser. All other parser still have the "default" value. (I don't think this feature is necessary)

I like this option because it provides default values while still allowing the user to change it like he wants. I for example use the library function and if i update the ebooks and there are mostly only one or two new chapters i don't need a slow down for the site. (I know edge case...)

I would argue that these options are under "Advanced Options" and if user break things because they configure it to fast for the website (HTTP 426) they are unlucky the same as when you use your cli with sudo or admin rights and you make a mistake.

@dteviot
Copy link
Owner

dteviot commented Sep 11, 2024

@gamebeaker
Obviously I wasn't clear.
What I meant is that currently, when you make a change, it's applied to all sites. e.g.

a.com is able to handle 8 FetchSimultaneously with 0 sec/ wait. The user changes the options accordingly. -> Now all sites have these new option if they aren't constrained in the parser through clampSimultanousFetchSize() and minimumThrottle (your first scenario). But if the user now changes these options for b.com to 4 FetchSimultaneously and 3 sec/ wait it also changes for a.com to the same options

What I think should happen (where we want to be) is when the settings are changed, the change only applies to the current site/parser. i.e. The user should not need to manually change the speed settings each time they want to download from a different site.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

2 participants