RUM-17182: Stablize MoveDataMigrationOperationTest under high CPU usage#3614
Conversation
| var lastAttemptEndNs = timeProvider.getDeviceElapsedTimeNanos() | ||
| while (attempt <= times) { | ||
| if (attempt > 1) { | ||
| val timeSinceLastAttemptNs = timeProvider.getDeviceElapsedTimeNanos() - lastAttemptEndNs | ||
| val timeToWaitNs = retryDelayNs - timeSinceLastAttemptNs | ||
| if (timeToWaitNs > 0) { | ||
| sleepSafe(TimeUnit.NANOSECONDS.toMillis(timeToWaitNs), internalLogger) | ||
| } | ||
| } |
There was a problem hiding this comment.
time provider is only really for testing and does complicate the code, is it worth removing?
There was a problem hiding this comment.
TimeProvider had a value for testing before because we weren't sleeping, so we could speed up the tests. With the new implementation (which I think is nice because we don't waste CPU), I guess what you are suggesting is basically removing all the timeToWait stuff and just sleeping for the whole delay.
I don't see much value from timeProvider now, so it makes sense to me to remove it.
There was a problem hiding this comment.
Yep, 100% what I was suggesting and drew same conclusion as me
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0cb1a187c3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
583b2f6 to
a1d3df9
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a1d3df92b3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
* Increase CPU time budget for when under high CPU load * Combine retryWithDelay overloads * Add a sleep into retryWithDelay to avoid spinning the CPU for the delay time
a1d3df9 to
6ddcc99
Compare
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## develop #3614 +/- ##
===========================================
+ Coverage 72.80% 72.85% +0.05%
===========================================
Files 975 975
Lines 35282 35271 -11
Branches 5973 5974 +1
===========================================
+ Hits 25684 25694 +10
+ Misses 7919 7906 -13
+ Partials 1679 1671 -8 🚀 New features to boost your workflow:
|
| var lastAttemptEndNs = timeProvider.getDeviceElapsedTimeNanos() | ||
| while (attempt <= times) { | ||
| if (attempt > 1) { | ||
| val timeSinceLastAttemptNs = timeProvider.getDeviceElapsedTimeNanos() - lastAttemptEndNs | ||
| val timeToWaitNs = retryDelayNs - timeSinceLastAttemptNs | ||
| if (timeToWaitNs > 0) { | ||
| sleepSafe(TimeUnit.NANOSECONDS.toMillis(timeToWaitNs), internalLogger) | ||
| } | ||
| } |
There was a problem hiding this comment.
TimeProvider had a value for testing before because we weren't sleeping, so we could speed up the tests. With the new implementation (which I think is nice because we don't waste CPU), I guess what you are suggesting is basically removing all the timeToWait stuff and just sleeping for the whole delay.
I don't see much value from timeProvider now, so it makes sense to me to remove it.
The TimeProvider does not offer much gain with the new retryWithDelay implementation that is not a spin-loop. This also un-weaves TimeProvider from callers since they are no longer needed
4 participants
What does this PR do?
Motivation
Tests failed locally when ran through local_ci.sh