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

feat(added cacheConfig in RequestDescriptor) #3070

Closed
wants to merge 9 commits into from

Conversation

morrys
Copy link
Contributor

@morrys morrys commented May 5, 2020

This change allows you to associate cache configurations directly in the request object.

In this way, once the operation has been created, it is possible to access all the information necessary to create a request in the network.

In addition, this modification allows you to add parameters to the CacheConfig object and easily use this information within Relay.

An example of a use case is to allow the configuration of a different TTL (bb88806) in the queries. So just add the TTL parameter inside CacheConfig and use that information inside the retain function without having to change its interface.

morrys added 5 commits May 5, 2020 17:40
This change allows you to associate cache configurations directly in the request object.

In this way, once the operation has been created, it is possible to access all the information necessary to create a request in the network.
…ction

if retry with a different cache configuration is requested, a new operation is created
@morrys
Copy link
Contributor Author

morrys commented Jun 23, 2020

hi @kassens,
any news or doubt about this PR? 👍

@morrys
Copy link
Contributor Author

morrys commented Jul 8, 2020

friendly ping @jstejada @josephsavona

@josephsavona
Copy link
Contributor

Overall this makes a lot of sense to me. I'm still skeptical of per-query TTL, but overall this could be a clean way to allow alternate store implementations to experiment with such features. My main concern is what to do about the RequestIdentifier, which is intended to identify and de-duplicate requests for the same information. There are various places in Relay where we de-duplicate queries based on this identifier, and we need to decide whether the CacheConfig should be part of that or not. I am inclined to agree w the implementation here and say no - the CacheConfig isn't part of the request id. But we need to think through this and make sure that doesn't cause problems for the intended use-case here.

Thoughts?

@morrys
Copy link
Contributor Author

morrys commented Jul 8, 2020

I am inclined to agree w the implementation here and say no

I am of the same opinion, because the request is always the same and leaving the identifier the same I have the possibility to recognize that the query is already retained in the store or it is still pending in the network

@morrys
Copy link
Contributor Author

morrys commented Aug 7, 2020

@josephsavona I just resolved the conflicts, any ETA on this?
thanks

@morrys
Copy link
Contributor Author

morrys commented Aug 27, 2020

friendly ping

@josephsavona I just resolved the conflicts, any ETA on this?
thanks

@josephsavona
Copy link
Contributor

cc @jstejada and @rbalicki2

Copy link
Contributor

@facebook-github-bot facebook-github-bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@rbalicki2 has imported this pull request. If you are a Facebook employee, you can view this diff on Phabricator.

@morrys
Copy link
Contributor Author

morrys commented Oct 16, 2020

Hi @rbalicki2, any update or ETA on this PR? thanks 👍

@facebook-github-bot
Copy link
Contributor

@rbalicki2 merged this pull request in eb256a3.

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

Successfully merging this pull request may close these issues.

3 participants