Remove activity definition cache #118
Conversation
| }, | ||
| new TemporalWorkerOptions().AddAllActivities(new DuplicateActivities("instance1"))); | ||
| } | ||
|
|
There was a problem hiding this comment.
Explaining this test to myself:
-
We define a workflow that returns a result containing the results of two activity calls.
-
The two activities are calls to the same activity method but on distinct instances (and using distinct task queues)
-
The utility
ExecuteWorkerAsyncallows us to start a worker and, while it's running, execute an arbitrary function that has access to the worker instance. We use it twice: -
The first usage starts the first worker and uses the function to make the second nested call to
ExecuteWorkerAsync -
The second usage starts a second worker and uses the function to execute our workflow and check that the result shows that distinct activity methods were invoked.
There was a problem hiding this comment.
So we're using the lambda passed to ExecuteWorkerAsync to essentially play the role of customer application (non-Worker) code, since it starts a workflow. Therefore it's a bit artificial that the function has access to an actual instance of the worker (this confused me initially). But it looks like the pass-a-function-that-receives-a-worker-instance is a deeper part of the non-test-code SDK design.
There was a problem hiding this comment.
Bingo. Before I removed the cache, this test would fail because the result of the workflow would be ["instance1","instance1"] because no matter which activity instance you gave to a worker, it used the first cached one based on method which was bad. Now it returns ["instance1","instance2"] as expected.
There was a problem hiding this comment.
So we're using the lambda passed to ExecuteWorkerAsync to essentially play the role of customer application (non-Worker) code, since it starts a workflow.
Yes
Therefore it's a bit artificial that the function has access to an actual instance of the worker (this confused me initially).
Mostly. It's just so I can get it to create that random task queue which I can use
But it looks like the pass-a-function-that-receives-a-worker-instance is a deeper part of the non-test-code SDK design.
Basically you have a worker that can run for some amount of time. In non-test code we support a worker running forever, until cancellation token is triggered, and/or until the completion of an async function. In test code, I made ExecuteWorkerAsync which is just a shortcut for the latter that also adds a workflow and makes a random task queue and does a couple of other things and gives the worker as the parameter just in case the test function wants to do something with it (most just use the task queue).
| }), | ||
| }; | ||
| } | ||
| } |
There was a problem hiding this comment.
Why does this test use two workers with different task queues?
There was a problem hiding this comment.
It only needs the activities on different workers not really the workflow (but I just reuse the execute worker helper). We use two workers to confirm that the same activity definition but with different instances for different workers returns different values.
So basically this workflow calls the same activity on different task queues and the test confirms that the activity called was the different instances on different task queues.
There was a problem hiding this comment.
It feels to me that the assertion being made is about what happens when we have distinct activity method instances and so it's not obvious that testing it requires multiple workers and task queues.
There was a problem hiding this comment.
The test is specifically "what happens when a workflow calls the same activity method but on different task queues where the different activity instances may have different state". This requires multiple task queues (task queues are 1:1 with workers, so it requires multiple workers).
| }, | ||
| new TemporalWorkerOptions().AddAllActivities(new DuplicateActivities("instance2"))); | ||
| }, | ||
| new TemporalWorkerOptions().AddAllActivities(new DuplicateActivities("instance1"))); |
There was a problem hiding this comment.
The way this test is written in a point-free style makes it a bit hard to read because the test assertion makes reference to "instance1" and "instance2" before the reader has encountered them.
There was a problem hiding this comment.
That's the consequence of lambdas. Parameters to methods accepting lambdas are invoked and often used before the lambda is invoked. We make the parameters after the lambda because they are optional and many don't use them. This is test code so it is an acceptable tradeoff IMO.
What was changed
Removed the activity definition cache. This cache was only useful in the past, before we used lambda expressions, to be able to repeatedly get the name and types at the call site. But now with lambda expressions we just get the name and assume that a compiled lambda means arg/result types are accurate. Also, the cache wasn't changed when we started storing custom invokers so the instance from the first activity definition was inadvertently reused.
Checklist