Skip to content

Priority annotations #274

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

Merged
merged 4 commits into from
May 29, 2025
Merged

Priority annotations #274

merged 4 commits into from
May 29, 2025

Conversation

Sushisource
Copy link
Member

What was changed

Updated Core
Expose priority setting on workflow client & in child/activities launched from workflows

Why?

Priorities are cool!

Checklist

  1. Closes [Feature Request] Support Priority Annotations #210

  2. How was this tested:
    Added integ test

  3. Any docs updates needed?

@Sushisource Sushisource requested a review from a team as a code owner May 28, 2025 23:04
@@ -38,6 +39,8 @@ module Activity
# @return [Float, nil] Heartbeat timeout set by the caller.
# @!attribute local?
# @return [Boolean] Whether the activity is a local activity or not.
# @!attribute priority
# @return [Priority, nil] The priority of this activity.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can this ever be nil? (same for workflow info)

Comment on lines 40 to 41
:priority,
:versioning_override,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
:priority,
:versioning_override,
:versioning_override,
:priority,

Pedantic, but might as well keep in the same order as the parameters of the outer call

@@ -70,6 +70,7 @@ def start_workflow(input)
input.static_summary, input.static_details, @client.data_converter
),
header: ProtoUtils.headers_to_proto(input.headers, @client.data_converter),
priority: input.priority&._to_proto,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
priority: input.priority&._to_proto,
priority: input.priority._to_proto,

Same for everywhere else where priority actually can't be nil

# The overall semantics of Priority are:
# 1. First, consider "priority_key": lower number goes first.
# (more will be added here later).
class Priority
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if this should be a Data class (and get all of the goodies it provides). I think it makes sense like record did for .NET and @dataclass did for Python. Still will need much of what you have here (so after the Data.define, re-open the class Priority to add stuff).

@@ -148,12 +149,14 @@ def self.execute_activity(
cancellation: Workflow.cancellation,
cancellation_type: ActivityCancellationType::TRY_CANCEL,
activity_id: nil,
disable_eager_execution: false
disable_eager_execution: false,
priority: Priority.default
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we add the import to this file and to activity.rb? I know it doesn't make sense to add everywhere, but along with temporalio/client, these two are kinda "single require entry points" to the SDK.

Copy link
Member

@cretz cretz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nothing blocking

require 'temporalio/api'

module Temporalio
Priority = Data.define(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If you want to default a field for this you can still leave the initialize the way you had it in the class and just call super, e.g.

def initialize(priority_key: nil)
  super
end

But requiring it is fine too.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, honestly requiring it makes more sense

class Priority
attr_reader priority_key: Integer?

def initialize: (?priority_key: Integer?) -> void
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
def initialize: (?priority_key: Integer?) -> void
def initialize: (priority_key: Integer?) -> void

Pedantic though

@@ -10,6 +10,7 @@ module Temporalio
attr_reader last_result: Object?
attr_reader namespace: String
attr_reader parent: ParentInfo?
attr_reader priority: Temporalio::Priority?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
attr_reader priority: Temporalio::Priority?
attr_reader priority: Temporalio::Priority

@@ -8,6 +8,7 @@ module Temporalio
attr_reader heartbeat_details: Array[Object?]
attr_reader heartbeat_timeout: Float?
attr_reader local?: bool
attr_reader priority: Temporalio::Priority?
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
attr_reader priority: Temporalio::Priority?
attr_reader priority: Temporalio::Priority

@Sushisource Sushisource enabled auto-merge (squash) May 29, 2025 21:08
@Sushisource Sushisource merged commit 196e0ac into main May 29, 2025
12 checks passed
@Sushisource Sushisource deleted the priority-annotations branch May 29, 2025 21:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Feature Request] Support Priority Annotations
2 participants