More Fuzzy Features #41
Conversation
Is it possible to add a scenario+SDK skip somehow? |
| updates: list[SearchAttributeUpdate[Any]] = [] | ||
| # TODO: Use RawValue after https://github.com/temporalio/sdk-python/issues/438 | ||
| # and avoid checking key by name | ||
| for k, v in action.upsert_search_attributes.search_attributes.items(): |
There was a problem hiding this comment.
We need to include the search attribute type with each search attribute item in this action IMO. Nowadays, search attribute setting needs to have key, value, and type every time to avoid ambiguity only the server can resolve.
There was a problem hiding this comment.
You mean add the type to the proto? Could do that. Testing all those isn't really that important here though, which is why I wanted to just pass raw payload through, since we're not asserting anything equals anything else really, just sometimes generating one of these commands.
There was a problem hiding this comment.
One of the primary reasons we did typed search attributes is because nowadays, we require more than just key+value to set a search attribute. We require key+type+value.
There was a problem hiding this comment.
Just going to keep using the string check since the attrs have to be predefined and doing this is essentially the same amount of work & the generator/dsl doesn't actually care about different types.
Turns out it doesn't matter anyway since we're not running full fuzzer in CI yet. |
| } | ||
| return nil, withAwaitableChoice(ctx, func(ctx workflow.Context) workflow.Future { | ||
| fut, setter := workflow.NewFuture(ctx) | ||
| workflow.Go(ctx, func(ctx workflow.Context) { |
There was a problem hiding this comment.
Why not just use a timer?
There was a problem hiding this comment.
Instead of what? The timer needs to be cancellable somehow and the only way to do that in conjunction with the different awaitable choices is to put it in another routine
There was a problem hiding this comment.
Instead of this sleep in a goroutine, it is very confusing to read when you can just create a timer and treat it's future like an activity future.
There was a problem hiding this comment.
The timer doesn't have a future is the problem
71ec0e4 to
144199a
Compare
| 0 => action::Variant::Timer(u.arbitrary()?), | ||
| 1 => action::Variant::ExecActivity(u.arbitrary()?), | ||
| _ => unreachable!(), | ||
| let action_kind = u.int_in_range(0..=100)? as f32; |
There was a problem hiding this comment.
Using 100 here is failing to honor non-integer chance percentages -- our default implementation of ActionChances uses values like 2.5 and 12.5, but these are effectively going to be clamped downwards. How about picking an integer between 0..=1000 and dividing by 10?
Also, the implementation below is worst case O(num_action_types^2), but we can do it in O(num_action_types). Instead of walking through the list of action probabilities for each action type to see if we're going to pick it, we can just walk through the probabilities once and emit the first type for which our random uniform value falls within the interval.
There was a problem hiding this comment.
Good points, though neither is I think is hugely important. I'll fix the clamping thing - the choosing thing maybe not just b/c the performance isn't an issue.
There was a problem hiding this comment.
Yes, the probability clamping should be fixed I think, but agree that from a performance POV the other change wouldn't affect any workloads we have planned. It should allow us to get rid of the macro-generated methods though, which is always a bit of a readability boost.
What was changed
Implement more fuzzy workflow features
Unfortunately we need temporalio/sdk-python#440 released or need to temporarily depend on an unreleased version for this to work.
Why?
Checklist
Closes
How was this tested: