Prevent None current_utc_time values when timestamps are really close to one another
#246
Conversation
ConnorMcMahon
left a comment
ConnorMcMahon
left a comment
There was a problem hiding this comment.
I think it's pretty close, but I have a few suggestions to make the code a bit more clear, at least from my perspective.
| outputs.append(task2) | ||
| outputs.append(task3) | ||
|
|
||
| return outputs |
There was a problem hiding this comment.
This is a good test, but you can make it even better by make sure that the value of current_utc_datetime is different for each of the calls you make. I wrote a similar test just the other day for the SQL backend and it uncovered a bunch of issues. Here is a reference to it if you think it might be useful to translate into Python: https://github.com/cgillum/durabletask-sqlserver/blob/a1a308c14b13ffca6a751f9fcdce111d55f40336/test/DurableTask.SqlServer.Tests/Integration/Orchestrations.cs#L114-L142
There was a problem hiding this comment.
I see. I want to take a step back to sync on the context here though as I think our tests are validating different scenarios.
Mine makes sure that, in the case that we cannot distinguish the timestamps of two OrchestratorStarted events (maybe they occur too close to one another), current_utc_datetime would not be None. I was testing this specifically because, only in this scenario, current_utc_datetime would get explicitly assigned to None. In this case contrived case, current_utc_datetime would never update :P
I think yours is testing that the deterministic time API is correctly updating, right? I do think that's a good test, so I'll add it :) but I just wanted to make sure we were on the same page as to what I was trying to test for.
There was a problem hiding this comment.
I understand it's beyond the scope of your PR, but given the fact that you decided to name this function generator_function_deterministic_utc_time I thought the suggestion for how you could also test for determinism might be appropriate. Of course, there would also need to be the None checks too since that's ultimately what you're trying to fix.
| if len(decision_started_events) != 0: | ||
| self.durable_context.decision_started_event = decision_started_events[0] | ||
|
|
||
| self.durable_context.current_utc_datetime = \ |
There was a problem hiding this comment.
I think this effectively doesn't do anything if len(decision_started_events) == 0. If that is the case, I would prefer we move this inside of the if block.
|
Applied both of y'alls feedback. Thanks! |
Addresses: #241
When orchestrator started events occur really close to one another, python's deterministic time API would sometimes return
None. This PR is designed to match the JS implementation and instead return the previous timestamp