Skip to content
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

P-150 feat: unit test for threadpool behaviour #2186

Merged
merged 20 commits into from
Nov 20, 2023
Merged

P-150 feat: unit test for threadpool behaviour #2186

merged 20 commits into from
Nov 20, 2023

Conversation

felixfaisal
Copy link
Member

Resolves #2165

Some notes:

  1. I had to implement our own AuthorApiMock, so that watch_top sends the trusted operation via a channel and we can use this channel to assert the order of operations.
  2. I've introduced a hardcoded sleep into assertion A1 under the test feature to create artificial lag during testing so that I can demonstrate the expected behaviour of threadpool
  3. This can be made even more modular to perform more tests but it could be a different issue

@felixfaisal felixfaisal marked this pull request as ready for review October 10, 2023 07:40
@felixfaisal felixfaisal requested a review from a team October 10, 2023 07:41
@Kailai-Wang
Copy link
Collaborator

Can you try to link the Linear issue too :)

@felixfaisal felixfaisal changed the title feat: unit test for threadpool behaviour P-60 feat: unit test for threadpool behaviour Oct 10, 2023
@felixfaisal felixfaisal changed the title P-60 feat: unit test for threadpool behaviour P-150 feat: unit test for threadpool behaviour Oct 10, 2023
@linear
Copy link

linear bot commented Oct 10, 2023

P-150 Create artificial lag for testing threadpool behavior

Context

This was mentioned in #2159
We need to introduce artificial lag onto the system and see if behaves correctly based on threadpools, One way would be having a ThrottledRequestType where the handling logic simply sleeps for X seconds before it returns.

@litentry litentry deleted a comment from linear bot Oct 10, 2023
Copy link
Collaborator

@Kailai-Wang Kailai-Wang left a comment

Choose a reason for hiding this comment

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

Thanks!

The logic looks fine, I've got two questions:

  • can we simply implement watch_top in AuthorApiMock? We are free to change it :) - to avoid repeating the code
  • can we have a test case where we have more than n_workers tasks (can be delayed A1) and see them returning in batch? Like the n_workers + 1th task should be processed only after we have vacant thread

Copy link
Member

@kziemianek kziemianek left a comment

Choose a reason for hiding this comment

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

Looks good in general. I left two comments. I think the one regarding feature toggle needs to be addresed at least.

@Kailai-Wang
Copy link
Collaborator

We need to resolve the conflict and then we are good to go I guess?

Copy link
Collaborator

@BillyWooo BillyWooo left a comment

Choose a reason for hiding this comment

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

Let's solved the conflicts and merge.

@felixfaisal felixfaisal merged commit 8552258 into dev Nov 20, 2023
26 checks passed
@Kailai-Wang Kailai-Wang deleted the fx-2165 branch January 1, 2024 09:06
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.

Create artificial lag for testing threadpool behavior
4 participants