Priorities for Workflows/Activities #2453
Conversation
|
Do we need test server support? and if so what level of support? |
I don't know, that's a question for the broader team I think. I would kind of hope not, since priority management seems like something you ought not to be dependent on for functional testing. |
| import java.util.Objects; | ||
|
|
||
| /** | ||
| * Priority contains metadata that controls the relative ordering of task processing when tasks are |
There was a problem hiding this comment.
I updated the text in the api repo slightly, not terribly important but you might want to grab the latest version just for consistency
cretz
left a comment
There was a problem hiding this comment.
LGTM and is nice and simple. I understand the server implementation is fairly new, but would be nice to see what an end-to-end test may look like so that other langs can add it too. I think a simple test is:
With worker off, start 10 workflows with priority being maybe workflow 5 first, then 7, then 3, then the rest. Then start worker with only a single workflow task slot (so cache disabled) and confirm that 5, 7, and 3 are first in that order. It's a simple test that will give SDKs peace of mind in server presence/impl.
Also, we should discuss whether we should open an issue for test server. Obviously if the logic is complicated we will just not implement and not take priority into account, but if it's as simple as changing the TaskQueues LinkedList to be a SortedSet, we should consider it.
temporal-sdk/src/main/java/io/temporal/internal/common/PriorityUtils.java
Show resolved
Hide resolved
|
|
||
| /** Return the priority of the activity task. */ | ||
| @Experimental | ||
| Priority getPriority(); |
There was a problem hiding this comment.
What situations is this nullable? Old servers only or always unless explicitly set? (just curious, though if confusing we should consider documenting the cases where it may be)
Also a bit confused on this class how a user can know whether a return is nullable or not. But given the confusion, best to be safe and make it clear in docs. I would consider Optional<Priority> if it can be nullable even on newer servers, but meh. Note this same concern does apply as much to user-set values like the activity options.
There was a problem hiding this comment.
If it can be nullable we should bias towards tagging with @Nullable vs optional, it is the preferred approach in Java
There was a problem hiding this comment.
It can be nullable insofar as you might be using an old server. So, yeah I'll attach the annotation. I believe new servers will always attach the (default) value.
There was a problem hiding this comment.
Would be worth documenting that and verifying the test server does match the real server here
There was a problem hiding this comment.
So server leaves this as null if unset which is the equivalent of default. In Go, if the server left it as null, we return the default object so I did the same here
There was a problem hiding this comment.
I think we need to not do that as it will lie. The default values for priorty are not 0, and therefore we might lie and seem like they've been set to zero for a server that actually just doesn't understand priority
There was a problem hiding this comment.
Zero means default, so zero is not a lie
See https://github.com/temporalio/temporal/blob/24cc4dd82/tests/priority_fairness_test.go for an sdk-based test. That uses activities but it would work with workflows too. The catch is that order is only approximate due to partitions, so use a larger number and check that ordering is within a threshold. Or force a single partition. |
cretz
left a comment
There was a problem hiding this comment.
Nothing blocking. Only marking approved now because I see the priority test which is great in that it fails currently when running against real server so I can have confidence this won't be merged until it is in a real server (i.e. a user can actually use it).
| */ | ||
| @Experimental | ||
| public class Priority { | ||
| public static Priority.Builder newBuilder() { |
There was a problem hiding this comment.
Wonder if we should have a create static helper that accepts the int and returns a Priority instance to make it easy for users
There was a problem hiding this comment.
We don't have helpers for any other builders, I don't think priority is special
1038984 to
86a1864
Compare
What was changed
Added priority annotations to starting workflows & activities
Why?
New feature!
Checklist
Closes Support Priority Annotations #2399
How was this tested:
Note
No tests yet. Ideally will write integ tests if we make a CLI RC
Will need docs for Priority generally