feat(task-processor): Add priority support #2847

gagantrivedi · 2023-10-13T05:52:56Z

Thanks for submitting a PR! Please check the boxes below:

I have run pre-commit to check linting
I have filled in the "Changes" section below?
I have filled in the "How did you test this code" section below?
I have used a Conventional Commit title for this Pull Request

Changes

In order to allow high priority tasks to be executed first, this PR adds a priority column to the task table(with default null)

How did you test this code?

Update unit test

vercel · 2023-10-13T05:53:04Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Comments	Updated (UTC)
docs	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Oct 24, 2023 10:28am
flagsmith-frontend-preview	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Oct 24, 2023 10:28am
flagsmith-frontend-staging	✅ Ready (Inspect)	Visit Preview	💬 Add feedback	Oct 24, 2023 10:28am

github-actions · 2023-10-13T05:57:23Z

Uffizzi Preview deployment-38400 was deleted.

codecov-commenter · 2023-10-13T06:16:04Z

Codecov Report

Attention: 2 lines in your changes are missing coverage. Please review.

Comparison is base (4f7464b) 95.59% compared to head (f951507) 95.64%.
Report is 23 commits behind head on main.

❗ Current head f951507 differs from pull request most recent head 5887a05. Consider uploading reports for the commit 5887a05 to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2847      +/-   ##
==========================================
+ Coverage   95.59%   95.64%   +0.04%     
==========================================
  Files        1009     1011       +2     
  Lines       28833    28979     +146     
==========================================
+ Hits        27562    27716     +154     
+ Misses       1271     1263       -8

Files	Coverage Δ
api/audit/tasks.py	`94.44% <100.00%> (+0.10%)`	⬆️
api/core/migration_helpers.py	`94.87% <100.00%> (+0.75%)`	⬆️
api/edge_api/identities/edge_request_forwarder.py	`100.00% <100.00%> (ø)`
api/edge_api/identities/tasks.py	`97.77% <100.00%> (+0.05%)`	⬆️
api/environments/tasks.py	`100.00% <100.00%> (ø)`
api/task_processor/decorators.py	`100.00% <100.00%> (+11.59%)`	⬆️
...igrations/0008_add_get_task_to_process_function.py	`100.00% <ø> (ø)`
...pi/task_processor/migrations/0010_task_priority.py	`100.00% <100.00%> (ø)`
...tions/0011_add_priority_to_get_tasks_to_process.py	`100.00% <100.00%> (ø)`
api/task_processor/models.py	`95.61% <100.00%> (+0.20%)`	⬆️
... and 4 more

... and 15 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

gagantrivedi · 2023-10-16T06:06:53Z

api/task_processor/models.py

@@ -90,12 +101,14 @@ class Meta:
    def create(
        cls,
        task_identifier: str,
+        priority: TaskPriority = TaskPriority.NORMAL,


@matthewelwell think we can get rid of schedule_task and just use this method directly?

We can, but I quite like having the helper method to differentiate them. What's the benefit in removing it?

I think the name is slightly odd, no? I'd expect it to schedule the task(i.e: put on some queue?) but it only creates the class instance. Does that make sense?

I see, you mean that it doesn't actually persist it to the DB? But that's the same as the create method too, right? We could update the name to create_scheduled_task ? Maybe you're right though that we should consolidate the 2 methods.

you mean that it doesn't actually persist it to the DB?

Yes, exactly

I've removed schedule_task method, but now I am not sure if it looks any better 😕 ?

Well, there's less code so that's always a good thing. I think I would prefer to keep the logic regarding the queue in the Task model though. Can we just handle that in the create method now instead?

api/task_processor/migrations/sql/0011_get_tasks_to_process.sql

matthewelwell

I've added a couple of minor comments.

This certainly seems like a ok approach but did we consider any other approaches here? For example, having distinct queues and the ability to define workers for those queues directly?

I guess we could still define a number of workers that operate only on a given priority if we wanted to, right?

matthewelwell · 2023-10-16T10:18:36Z

api/edge_api/identities/tasks.py

@@ -71,7 +72,7 @@ def call_environment_webhook_for_feature_state_change(
    call_environment_webhooks(environment, data, event_type=event_type)


-@register_task_handler()
+@register_task_handler(priority=TaskPriority.HIGHEST)


Does this need to be highest? I think we basically want to reserve highest for those tasks involved in creating the environment document, right?

Think this is as import as environment document, right? We want the identity overrides to reflect as soon as possible

I'd say it's slightly lower priority than environment document changes.

matthewelwell · 2023-10-16T10:29:55Z

api/task_processor/models.py

@@ -90,12 +101,14 @@ class Meta:
    def create(
        cls,
        task_identifier: str,
+        priority: TaskPriority = TaskPriority.NORMAL,


We can, but I quite like having the helper method to differentiate them. What's the benefit in removing it?

matthewelwell · 2023-10-23T16:02:53Z

api/core/migration_helpers.py

-        with open(file_path) as f:
-            return cls(f.read(), reverse_sql=reverse_sql)
+    def from_sql_file(
+        cls, file_path: str, reverse_sql: str = None, reverse_sql_file_path: str = None


We should probably add validation here to ensure that reverse_sql and reverse_sql_file_path aren't used together? I guess an alternative would be to use a single argument and try to infer if it's a file path (e.g. os.file.exists()).

api/task_processor/migrations/0011_add_priority_to_get_tasks_to_process.py

matthewelwell · 2023-10-23T16:14:51Z

api/task_processor/models.py

@@ -90,12 +101,14 @@ class Meta:
    def create(
        cls,
        task_identifier: str,
+        priority: TaskPriority = TaskPriority.NORMAL,


Well, there's less code so that's always a good thing. I think I would prefer to keep the logic regarding the queue in the Task model though. Can we just handle that in the create method now instead?

matthewelwell

Added a few minor comments but this looks good. It relies on us clearing down the taskprocessor_task table before we can release though.

matthewelwell · 2023-10-24T09:43:57Z

api/task_processor/models.py

+        if queue_size:
+            if cls.is_queue_full(task_identifier, queue_size):


Nitpick:

Suggested change

if queue_size:

if cls.is_queue_full(task_identifier, queue_size):

if queue_size and cls.is_queue_full(task_identifier, queue_size):

matthewelwell · 2023-10-24T09:44:45Z

api/task_processor/models.py

-            task_identifier=task_identifier,
-            args=args,
-            kwargs=kwargs,
+    def is_queue_full(cls, task_identifier: str, queue_size: int) -> bool:


This can probably be marked as private now, right?

Suggested change

def is_queue_full(cls, task_identifier: str, queue_size: int) -> bool:

def _is_queue_full(cls, task_identifier: str, queue_size: int) -> bool:

gagantrivedi added 5 commits October 10, 2023 16:57

Add priority

7afa715

rename sql functions

37bcce3

fixup! add choices for priority

8f1b786

update sql function to use priority

445ece7

Add test for priority order

2e02a25

github-actions bot added the api Issue related to the REST API label Oct 13, 2023

gagantrivedi marked this pull request as draft October 13, 2023 05:53

squash! regenerate migration

f7202ba

vercel bot deployed to Preview – flagsmith-frontend-preview October 13, 2023 06:06 View deployment

vercel bot deployed to Preview – flagsmith-frontend-staging October 13, 2023 06:08 View deployment

vercel bot deployed to Preview – docs October 13, 2023 06:08 View deployment

Add priority to relevant tasks

a7df921

vercel bot deployed to Preview – flagsmith-frontend-staging October 16, 2023 05:31 View deployment

vercel bot deployed to Preview – flagsmith-frontend-preview October 16, 2023 05:33 View deployment

vercel bot deployed to Preview – docs October 16, 2023 05:34 View deployment

gagantrivedi marked this pull request as ready for review October 16, 2023 06:04

gagantrivedi requested review from a team, novakzaballa and matthewelwell October 16, 2023 06:04

gagantrivedi commented Oct 16, 2023

View reviewed changes

api/task_processor/migrations/sql/0011_get_tasks_to_process.sql Show resolved Hide resolved

gagantrivedi marked this pull request as draft October 16, 2023 06:20

gagantrivedi marked this pull request as ready for review October 16, 2023 08:29

matthewelwell reviewed Oct 16, 2023

View reviewed changes

fix migration reverse

f25c2ac

vercel bot deployed to Preview – flagsmith-frontend-staging October 17, 2023 05:08 View deployment

vercel bot deployed to Preview – flagsmith-frontend-preview October 17, 2023 05:10 View deployment

vercel bot deployed to Preview – flagsmith-frontend-preview October 20, 2023 07:33 View deployment

refac: remove schedule_task classmethod

1bfe023

vercel bot deployed to Preview – flagsmith-frontend-preview October 20, 2023 08:23 View deployment

vercel bot deployed to Preview – docs October 20, 2023 08:23 View deployment

vercel bot deployed to Preview – flagsmith-frontend-staging October 20, 2023 08:25 View deployment

gagantrivedi requested a review from matthewelwell October 23, 2023 10:19

gagantrivedi assigned matthewelwell Oct 23, 2023

matthewelwell reviewed Oct 23, 2023

View reviewed changes

gagantrivedi added 3 commits October 24, 2023 09:37

remove comment

fa5ea12

Add check for from_sql_file method

802a4f8

improve from_sql_file add tests

ad8b538

vercel bot deployed to Preview – docs October 24, 2023 05:40 View deployment

vercel bot deployed to Preview – flagsmith-frontend-staging October 24, 2023 05:42 View deployment

vercel bot deployed to Preview – flagsmith-frontend-preview October 24, 2023 05:44 View deployment

gagantrivedi added 2 commits October 24, 2023 11:35

move queue size check to create

1b53f06

add tests

f951507

vercel bot deployed to Preview – flagsmith-frontend-preview October 24, 2023 07:12 View deployment

vercel bot deployed to Preview – docs October 24, 2023 07:12 View deployment

vercel bot deployed to Preview – flagsmith-frontend-staging October 24, 2023 07:14 View deployment

gagantrivedi requested a review from matthewelwell October 24, 2023 07:16

matthewelwell approved these changes Oct 24, 2023

View reviewed changes

refac: make is_queue_full private

5887a05

vercel bot deployed to Preview – docs October 24, 2023 10:25 View deployment

vercel bot deployed to Preview – flagsmith-frontend-staging October 24, 2023 10:26 View deployment

vercel bot deployed to Preview – flagsmith-frontend-preview October 24, 2023 10:28 View deployment

matthewelwell added this pull request to the merge queue Oct 31, 2023

Merged via the queue into main with commit 6830ef6 Oct 31, 2023

matthewelwell deleted the feat/priority-task branch October 31, 2023 10:20

dabeeeenster mentioned this pull request Oct 31, 2023

chore(main): release 2.78.0 #2904

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(task-processor): Add priority support #2847

feat(task-processor): Add priority support #2847

gagantrivedi commented Oct 13, 2023 •

edited

Loading

vercel bot commented Oct 13, 2023 •

edited

Loading

github-actions bot commented Oct 13, 2023 •

edited

Loading

codecov-commenter commented Oct 13, 2023 •

edited

Loading

gagantrivedi Oct 16, 2023

matthewelwell Oct 16, 2023

gagantrivedi Oct 17, 2023

matthewelwell Oct 17, 2023

gagantrivedi Oct 18, 2023 •

edited

Loading

gagantrivedi Oct 20, 2023 •

edited

Loading

matthewelwell Oct 23, 2023

gagantrivedi Oct 24, 2023

matthewelwell left a comment

matthewelwell Oct 16, 2023

gagantrivedi Oct 17, 2023

matthewelwell Oct 17, 2023 •

edited

Loading

matthewelwell Oct 16, 2023

matthewelwell Oct 23, 2023

matthewelwell Oct 23, 2023

matthewelwell left a comment

matthewelwell Oct 24, 2023

matthewelwell Oct 24, 2023

		if queue_size:
		if cls.is_queue_full(task_identifier, queue_size):

	if queue_size:
	if cls.is_queue_full(task_identifier, queue_size):
	if queue_size and cls.is_queue_full(task_identifier, queue_size):

	def is_queue_full(cls, task_identifier: str, queue_size: int) -> bool:
	def _is_queue_full(cls, task_identifier: str, queue_size: int) -> bool:

feat(task-processor): Add priority support #2847

feat(task-processor): Add priority support #2847

Conversation

gagantrivedi commented Oct 13, 2023 • edited Loading

Changes

How did you test this code?

vercel bot commented Oct 13, 2023 • edited Loading

github-actions bot commented Oct 13, 2023 • edited Loading

codecov-commenter commented Oct 13, 2023 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gagantrivedi Oct 18, 2023 • edited Loading

Choose a reason for hiding this comment

gagantrivedi Oct 20, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewelwell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewelwell Oct 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

matthewelwell left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gagantrivedi commented Oct 13, 2023 •

edited

Loading

vercel bot commented Oct 13, 2023 •

edited

Loading

github-actions bot commented Oct 13, 2023 •

edited

Loading

codecov-commenter commented Oct 13, 2023 •

edited

Loading

gagantrivedi Oct 18, 2023 •

edited

Loading

gagantrivedi Oct 20, 2023 •

edited

Loading

matthewelwell Oct 17, 2023 •

edited

Loading