Make Load all operations retryable [CTT-723] - #571
Conversation
|
@shultseva, could you please add a short explanation to the PR description covering why this change is needed? |
ahmetmircik
left a comment
ahmetmircik
left a comment
There was a problem hiding this comment.
LGTM, PR description can be improved.
| Loads all keys into the store. This is a batch load operation so that an implementation can optimize the multiple loads. | ||
| request: | ||
| retryable: false | ||
| retryable: true |
There was a problem hiding this comment.
Do we really need to set retryable to true?
Retries are not idempotent and may trigger duplicate invalidations, WAN updates, interceptor calls, and other side effects.
I see two options:
- Keep
retryableasfalseand document the recommended workaround. - Make the operation idempotent before enabling retries.
WDYT?
There was a problem hiding this comment.
Yes, if we want fault tolerance for client-side requests, we need retries.
For a full load, if the load is already in progress, another full load will not be started. If the previous load has already finished, then yes, another one will be triggered. But since it finished between retries, the load should be fast, so I don’t see a problem with triggering it twice.
For a partial load, yes, it can be triggered twice. So what? It will rewrite the same values (or do nothing if replace = false), but I still don’t see a problem with that.
There was a problem hiding this comment.
Yes, the main issue I see is a late retry overwriting a newer value with an older one. Some form of idempotency could prevent this; even a simple ordered UUID might be sufficient.
2 participants
Load all, Load all subsets are retryable now
Initial PR - https://github.com/hazelcast/hazelcast-mono/pull/4888