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

CA-404013: replace Thread.delay with Delay module #6213

Merged
merged 1 commit into from
Jan 8, 2025

Conversation

gangj
Copy link
Contributor

@gangj gangj commented Jan 7, 2025

Reporter.cancel would be blocked for a long time by backoff delay when another thread is waiting for next reading, replace Thread.delay with Delay module so that Reporter.cancel will not be blocked.

@last-genius last-genius added the blocker release blocker label Jan 7, 2025
Copy link
Contributor

@edwintorok edwintorok left a comment

Choose a reason for hiding this comment

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

@@ -57,10 +59,20 @@ type state =
| Cancelled
| Stopped of [`New | `Cancelled | `Failed of exn]
Copy link
Contributor

Choose a reason for hiding this comment

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

Unrelated - but I would have preferred this to be a regular sum type. This module needs to be in scope so I don't see the benefit to use a polymorphic sum type.

@robhoes
Copy link
Member

robhoes commented Jan 7, 2025

I don't see a problem with this patch, but there is a test failure that we need to understand before merging.

Reporter.cancel would be blocked for a long time by backoff delay when
another thread is waiting for next reading, replace Thread.delay with
Delay module so that Reporter.cancel will not be blocked.

Signed-off-by: Gang Ji <[email protected]>
@gangj gangj force-pushed the private/gangj/CA-404013 branch from 518ecec to 5efa5d0 Compare January 8, 2025 10:34
@gangj
Copy link
Contributor Author

gangj commented Jan 8, 2025

I don't see a problem with this patch, but there is a test failure that we need to understand before merging.

The failed test TCRrdLogSpam has comments: "rrdd-iostat shouldn't spam the logs", in the private build which failed the test, I added some logs in rrdd code.
Rerun the test 10 times with a private build without no new logs, all passed.

@gangj gangj added this pull request to the merge queue Jan 8, 2025
Merged via the queue into xapi-project:master with commit 264ca76 Jan 8, 2025
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
blocker release blocker
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants