search

temporalio / sdk-core

Core Temporal SDK that can be used as a base for language specific Temporal SDKs
MIT License
278 stars 77 forks source link

Fix removal of deprecated patch call adjacent to other patch #587

Closed Sushisource closed 1 year ago

Sushisource commented 1 year ago

What was changed

Permit the removal of deprecated patch calls which are immediately adjacent to another patch call.

This was done by allowing patch-id-mistmaches in when checking a patch event against a patch machine, if the event is deprecated. I do not see this as a risky change primarily because nondeterminism checking of this kind purely serves as an "early warning" against other subsequent nondeterminism which would happen as a result of taking the incorrect branch. So, although this change technically means you could swap out one deprecated patch for another and there would be no error on that line - you'd still likely encounter another one later as if you hadn't used patching at all.

I think this is an acceptable tradeoff, since I cannot think of any other way to make this work and at least a couple people have run into it.

Why?

Without this, it can be quite challenging to remove deprecated patches in some circumstances.

Checklist

  1. Closes https://github.com/temporalio/sdk-core/issues/535

  2. How was this tested: Added IT, existing tests.

  3. Any docs updates needed?

cretz commented 1 year ago

I admit not understanding the problem well enough (but my understanding may not be required, but if my understanding is required, problematic TS/Python code on patch change could help me). I'll defer review.

Permit the removal [...] I do not see this as a risky change [...]

Sounds like this is a harmless change simply because you're loosening restrictions, correct?

Sushisource commented 1 year ago

Sounds like this is a harmless change simply because you're loosening restrictions, correct?

Yes. I wouldn't call it completely harmless. The potential harm is in making the reason something has gone wrong somewhat less obvious - but it's not harmful from a "bad things will happen" perspective.