-
Notifications
You must be signed in to change notification settings - Fork 41
Fairness Keys & Weights #508
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
.github/workflows/ci.yml
Outdated
ClangSharpPInvokeGenerator @src/Temporalio/Bridge/GenerateInterop.rsp | ||
dotnet run --project src/Temporalio.Api.Generator | ||
npx doctoc README.md | ||
git add src/Temporalio/Api |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this leftover code from using CI to generate some aspect?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's not leftover, it's genuinely useful for getting protos from CI that are net-new files
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I forget the git diff
details, but do we also need to change git diff --exit-code
to be git diff --cached --exit-code
? The goal is to detect anything that doesn't match a re-run of the generator, including new files. Also, what about the other generated proto place, src/Temporalio/Bridge/Api
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added those 👍
public Priority(int? priorityKey = null) => PriorityKey = priorityKey; | ||
/// <param name="fairnessKey">The fairness key.</param> | ||
/// <param name="fairnessWeight">The fairness weight.</param> | ||
public Priority(int? priorityKey = null, string? fairnessKey = null, float? fairnessWeight = null) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We are inconsistent across languages on whether we require a priority key in the constructor or not
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah it's actually Ruby that was the odd one out here. Everywhere else is optional. Adding a fix here: temporalio/sdk-ruby#323
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Python also requires priority key be set when constructing, note how https://github.com/temporalio/sdk-python/blob/3c6fae6606428bb895c4a0df0c0a9c20c6e4bf82/temporalio/common.py#L985 doesn't have a default of None
Hmm, looks like there are some additional changes to the span expectations that are causing issues |
tests/Temporalio.Tests/AssertMore.cs
Outdated
foreach (var item in items) | ||
{ | ||
var found = false; | ||
var exceptions = new List<Exception>(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Technically no need for this collection anymore, but no big deal
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, derp
What was changed
Added fairness keys/weights to priority
Why?
New feature
Checklist
Closes
How was this tested:
Added test
Any docs updates needed?