-
Notifications
You must be signed in to change notification settings - Fork 240
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
fix: adds order by timestamp #4316
fix: adds order by timestamp #4316
Conversation
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## main #4316 +/- ##
==========================================
+ Coverage 71.74% 75.18% +3.43%
==========================================
Files 919 1052 +133
Lines 18457 21114 +2657
Branches 1037 1180 +143
==========================================
+ Hits 13242 15874 +2632
+ Misses 4756 4725 -31
- Partials 459 515 +56 ☔ View full report in Codecov by Sentry. |
extensions/control-plane/store/sql/contract-negotiation-store-sql/docs/schema.sql
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I also agree that the PolicyMonitor could be refactored into a watchdog, when it was implemented the state machine looked like the more natural thing to do, but in fact there's only one non final state (STARTED) so the SM is definitely overkill
* fix: adds order by timestamp + tests in state machines * pr remarks
What this PR changes/adds
Adds a deterministic order by
stateTimestamp
field innextNotLeased
method of state machines:PolicyMonitor
DataPlane
ContractNegotiation
TransferProcess
This will allow a consistent and fair processing order for entities based on their state.
By using an index on
state
+ timestamp it will also speedup processing states that need to transition and it will also lower down the usage of the database which we constantly poll for next states to process. State machine should not poll final state or states that needs to be monitored for a long period of time.While profiling state machine the
PolicyMonitor
was on of those which spikes the CPU usage of both connector anddatabase since basically it's constantly polling the database for the entries in
STARTED
state, which grows over time without transitioning. Also theedc.policy.monitor.state-machine.iteration-wait-millis
didn't help since the state machine does not wait if at least one entry has been process in a tick. To lower down the load to the database usage of thePolicyMonitor
state machine theprocessMonitoring
processor now returnfalse
if no state transition is made. In this way thewait
configuration can be applied.Why it does that
Briefly state why the change was necessary.
Further notes
We might think of reworking
PolicyMonitor
state machine, since it's a bit different from the other state machine and it needs constantly checking on transfer process states and policy evaluation. In the current form can be a CPU hogLinked Issue(s)
Closes #4308
Please be sure to take a look at the contributing guidelines and our etiquette for pull requests.