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

Handle network retries in fetching remote entries #83

Closed
ronaldtse opened this issue Feb 1, 2021 · 10 comments
Closed

Handle network retries in fetching remote entries #83

ronaldtse opened this issue Feb 1, 2021 · 10 comments
Assignees
Labels
enhancement New feature or request

Comments

@ronaldtse
Copy link
Contributor

Sometimes Relaton source data web sites are unreliable and may require retries.

@skalee suggests that Relaton can handle retries when fetching remote entries.

@andrew2net what do you think is the cleanest way to do so? I imagine we will need some configurable options, and we need to specify the timeouts/parallel/rate limits of each individual service so the user calls won't run into issues.

Originally from glossarist/iev-data#103.

@ronaldtse ronaldtse added the enhancement New feature or request label Feb 1, 2021
@ronaldtse
Copy link
Contributor Author

Please also refer to glossarist/iev-data#103 for an initial implementation.

@andrew2net
Copy link
Contributor

@ronaldtse we can implement it in the relaton gem, so we don't have to implement the functionality in each flavor gem but in this case using the flavor gems separately won't handle network retries.

@ronaldtse
Copy link
Contributor Author

@andrew2net yeah that's okay. However each flavor gem may have different endpoint limitations. How do we express them in the gems and ensure that the user complies with them? e.g. rate limit 6 per second, etc.

@andrew2net
Copy link
Contributor

However each flavor gem may have different endpoint limitations. How do we express them in the gems and ensure that the user complies with them? e.g. rate limit 6 per second, etc.

@ronaldtse we can set the information about limitations in the flavor gems and get it as need

@ronaldtse
Copy link
Contributor Author

Sounds good. Please help proceed, thanks!

@andrew2net
Copy link
Contributor

@ronaldtse the relaton gem is used to fetch documents one by one, so it can't use threads to fetch documents simultaneously. Network retries without parallel querying could significantly slow down fetching documents.
I think we need either implement fetching a bunch of references or use thread pool outside the relaton gem.

@ronaldtse
Copy link
Contributor Author

Perhaps it is wise to use a thread pool?

@andrew2net
Copy link
Contributor

Yes, thread poll is that we need to speed up fetching a bunch of documents. But for now, relaton has fetch method which accepts only one reference. A caller has to wait until the relaton returns a result before fetching the next reference. We need to change the way how the other scripts use the relaton.
I suggest we leave the "fetch" method as is, for compatibility with currently existed use cases, and implement a new method "queue_fetch" which doesn't return a result immediately but allow to request all needed reference then wait until the references fetching.
The question is, do we need to implement network retries for the current "fetch" method (it could dramatically slow down fetching in some cases)? Shouldn't we implement it only in the new "queue_fetch" method?

@skalee
Copy link

skalee commented Feb 10, 2021

How about adding retries keyword parameter with default value 0? It won't break existing users too, and I suppose that blocking #fetch will be much simpler to use than non-blocking #queue_fetch. Of course only multi-threaded clients will find this feature beneficial, but I guess this would be the case for other solutions too.

(Though I'm not sure yet if I'll be able to use that reliably. Jekyll doesn't seem to be multi-threaded. However #queue_fetch doesn't seem to be helpful in my case either.)

@ronaldtse
Copy link
Contributor Author

@skalee you have a good point. Agree with the treatment.

andrew2net added a commit that referenced this issue Feb 11, 2021
andrew2net added a commit that referenced this issue Feb 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

3 participants