Add a method to refresh only if lookup is a success, not an error. #159

kajebiii · 2023-09-22T02:43:36Z

Added def refreshValue(key: Key): IO[Error, Unit] to keep old value in case of an error.

CLAassistant · 2023-09-22T02:43:41Z

All committers have signed the CLA.

adamgfraser

There are a lot of concurrency problems here. Setting the value and then trying to set it back introduces significant complexity and issues. In particular, if we replace a MapValue.Pending then we must ensure that the promise is completed under all circumstances.

kajebiii · 2023-09-26T02:13:20Z

Thanks for your reply!

There are a lot of concurrency problems here. Setting the value and then trying to set it back introduces significant complexity and issues. In particular, if we replace a MapValue.Pending then we must ensure that the promise is completed under all circumstances.

Can you tell me some specific examples that are problematic?

ghostdogpr · 2023-09-26T02:14:50Z

We didn't change the promise completion logic, it will be same as before: when moving away from Pending, the promise is always completed.
The only change is what we set as the new value (map.put) after lookup completion, before it was the new exit, now it can be:

the new exit (old behavior when using refresh)
the old exit (when using refreshValue and there was a completed value before)
nothing (when using refreshValue and there was no completed value before)

adamgfraser · 2023-09-26T02:57:51Z

If I concurrently get a value immediately after the refresh has started and the computation fails I will observe the error, but if I get the value a moment later after the computation has failed I'll get the old value or potentially nothing at all.

ghostdogpr · 2023-09-26T03:22:10Z

If I concurrently get a value immediately after the refresh has started and the computation fails I will observe the error, but if I get the value a moment later after the computation has failed I'll get the old value or potentially nothing at all.

Calling get immediately after refreshValue:

If there was a value in the cache before
- calling get before lookup ends will get you the old value because the state is Refreshing
- after lookup ends
  - error case: we set the cache to the previous Completed state so get will still return the old value
  - success case: we set the cache to the new Completed state so get will now return the new value
If there was no value in the cache before
- calling get before lookup ends will wait on the promise because the state is Pending, the promise will complete with either the error or the success
- after lookup ends
  - error case: we remove the cache state so get will trigger another lookup
  - success case: we set the cache to the new Completed state so get will now return the new value

Only the part in bold is new, the rest is how refresh currently works.

adamgfraser · 2023-09-26T03:28:30Z

Sorry if I'm missing something here but currently if I'm waiting on the promise I either get the old value or I get the new value. But under this logic if I'm waiting on the promise the promise is being completed with the exit value of the workflow, even if it is an error. So now I have a case where this error value can be observed even though it is never being put in the cache. So before I either get old or new versus now I get old, new, or this ephemeral failure value.

adamgfraser · 2023-09-26T03:43:42Z

zio-cache/shared/src/main/scala/zio/cache/Cache.scala

@@ -256,7 +268,8 @@ object Cache {
                  value = map.putIfAbsent(k, MapValue.Pending(new MapKey(k), promise))
                }
                val result = if (value eq null) {
-                  lookupValueOf(in, promise)
+                  val rollbackResultIfError = if (rollbackIfError) Right(None) else Left(())


Not sure this really makes sense. If the value isn't in the cache at all how is this any different from a get which will put the value in the cache even if it is an error? I think one of the issues here is we are treating refresh inconsistently with get.

I see your point. To be consistent with get we can say that we put the error into the cache then and remove that special case.

Yeah I think our principle should be that the idea, as I understand it, is that if there is a valid "good" value in the cache we don't want to overwrite it with a "bad" value. One question that raises is what we should do when the original value is itself an error.

I do wonder whether this is the best path to address this. Like do you want to cache error values at all?

Empty cache

Lookup Success -> replace with lookup exit

Lookup Error -> replace with lookup exit

Existing cache value

Lookup Success -> replace with lookup exit

Lookup Error

if the old one was a success, we keep it

if the old one was an error, do we keep it or replace with the new error? Replacing will reset the error ttl, so I think keeping the old one would be better since the main goal is to refresh the success value?

adamgfraser · 2023-09-26T03:45:04Z

zio-cache/shared/src/main/scala/zio/cache/Cache.scala

+                      rollbackResultIfError match {
+                        case Left(()) =>
+                          map.put(key, MapValue.Complete(new MapKey(key), exit, entryStats, now.plus(timeToLive(exit))))
+                        case Right(None) =>


Similarly here this is basically a get when there is no previous value.

adamgfraser · 2023-09-26T03:45:46Z

zio-cache/shared/src/main/scala/zio/cache/Cache.scala

+                        case Right(None) =>
+                          map.remove(key)
+                        case Right(Some(rollbackResult)) =>
+                          map.put(key, rollbackResult)


Here if the time to live has already expired this is really another get.

ghostdogpr · 2023-09-26T05:03:41Z

Actually thinking more about this, there is another problem: if you call refresh while refreshValue is ongoing, it will do nothing.

Any ideas for a different approach? What really matters for us is only the case where we had a good value that we want to refresh it only with another good one. We don't really care of cases where the value didn't exist or was an error. A simple set would actually be enough as we could lookup ourselves and override it.

kajebiii · 2023-09-26T07:12:52Z

I made PR with a different approach.

Can you take a look at the above PR?

kajebiii added 5 commits September 21, 2023 14:55

Add rollbackIfFailed parameter in refresh

250a3ac

Implement another operator refreshValue

25c3933

Return exit on promise

481871e

Implement test about refreshValue

3df9381

Add case for rollback to empty cache

5da82d1

Fix typo

bc00941

adamgfraser reviewed Sep 26, 2023

View reviewed changes

kajebiii mentioned this pull request Sep 26, 2023

Add a parameter cacheValuesOnly to Cache.make.* #160

Closed

kajebiii closed this Oct 16, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add a method to refresh only if lookup is a success, not an error. #159

Add a method to refresh only if lookup is a success, not an error. #159

kajebiii commented Sep 22, 2023

CLAassistant commented Sep 22, 2023 •

edited

Loading

adamgfraser left a comment

kajebiii commented Sep 26, 2023 •

edited

Loading

ghostdogpr commented Sep 26, 2023

adamgfraser commented Sep 26, 2023 •

edited

Loading

ghostdogpr commented Sep 26, 2023

adamgfraser commented Sep 26, 2023

adamgfraser Sep 26, 2023

ghostdogpr Sep 26, 2023

adamgfraser Sep 26, 2023

adamgfraser Sep 26, 2023

ghostdogpr Sep 26, 2023

adamgfraser Sep 26, 2023

adamgfraser Sep 26, 2023

ghostdogpr commented Sep 26, 2023

kajebiii commented Sep 26, 2023

Add a method to refresh only if lookup is a success, not an error. #159

Add a method to refresh only if lookup is a success, not an error. #159

Conversation

kajebiii commented Sep 22, 2023

CLAassistant commented Sep 22, 2023 • edited Loading

adamgfraser left a comment

Choose a reason for hiding this comment

kajebiii commented Sep 26, 2023 • edited Loading

ghostdogpr commented Sep 26, 2023

adamgfraser commented Sep 26, 2023 • edited Loading

ghostdogpr commented Sep 26, 2023

adamgfraser commented Sep 26, 2023

adamgfraser Sep 26, 2023

Choose a reason for hiding this comment

ghostdogpr Sep 26, 2023

Choose a reason for hiding this comment

adamgfraser Sep 26, 2023

Choose a reason for hiding this comment

adamgfraser Sep 26, 2023

Choose a reason for hiding this comment

ghostdogpr Sep 26, 2023

Choose a reason for hiding this comment

adamgfraser Sep 26, 2023

Choose a reason for hiding this comment

adamgfraser Sep 26, 2023

Choose a reason for hiding this comment

ghostdogpr commented Sep 26, 2023

kajebiii commented Sep 26, 2023

CLAassistant commented Sep 22, 2023 •

edited

Loading

kajebiii commented Sep 26, 2023 •

edited

Loading

adamgfraser commented Sep 26, 2023 •

edited

Loading