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

feat: schedule create throttling #9994

Merged
merged 16 commits into from Nov 27, 2023
Merged

Conversation

MiroslavGatsanoga
Copy link
Collaborator

@MiroslavGatsanoga MiroslavGatsanoga commented Nov 20, 2023

Description:

Update mod implementation of the throttle facility to handle the case of ScheduleCreate transactions and also accumulate for the scheduled transaction within that ScheduleCreate.

Related issue(s):

Fixes #9798

Notes for reviewer:

The PR adds the ScheduleCreate & ScheduleSign related implementation in the existing throttle facility. What is still not implemented (to be addressed in the future):

Checklist

  • Documented (Code comments, README, etc.)
  • Tested (unit, integration, etc.)

Signed-off-by: Miroslav Gatsanoga <miroslav.gatsanoga@limechain.tech>
Signed-off-by: Miroslav Gatsanoga <miroslav.gatsanoga@limechain.tech>
Copy link

github-actions bot commented Nov 20, 2023

Node: Unit Test Results

    2 284 files  +  1      2 284 suites  +1   46m 56s ⏱️ -26s
118 382 tests +17  118 348 ✔️ +17  34 💤 ±0  0 ±0 
126 756 runs  +73  126 722 ✔️ +73  34 💤 ±0  0 ±0 

Results for commit 6facfa6. ± Comparison against base commit fd86222.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Nov 20, 2023

Node: E2E Test Results

    1 files      1 suites   22m 27s ⏱️
310 tests 310 ✔️ 0 💤 0
332 runs  332 ✔️ 0 💤 0

Results for commit 6facfa6.

♻️ This comment has been updated with latest results.

Copy link

github-actions bot commented Nov 20, 2023

Node: Integration Test Results

279 tests   279 ✔️  28m 14s ⏱️
    5 suites      0 💤
    5 files        0

Results for commit 6facfa6.

♻️ This comment has been updated with latest results.

Signed-off-by: Miroslav Gatsanoga <miroslav.gatsanoga@limechain.tech>
Signed-off-by: Miroslav Gatsanoga <miroslav.gatsanoga@limechain.tech>
Signed-off-by: Miroslav Gatsanoga <miroslav.gatsanoga@limechain.tech>
@MiroslavGatsanoga
Copy link
Collaborator Author

@jsync-swirlds regarding the Notes for reviewer section: should we create issues for those two items now or they are not planned to be part of the initial modularization release? cc @netopyr

Copy link

github-actions bot commented Nov 20, 2023

Node: HAPI Test Results

1 245 tests  ±0   968 ✔️ ±0   1h 36m 6s ⏱️ + 4m 10s
   166 suites ±0   277 💤 ±0 
   166 files   ±0       0 ±0 

Results for commit 2d5ddde. ± Comparison against base commit 1783f9d.

♻️ This comment has been updated with latest results.

@jsync-swirlds
Copy link
Member

@jsync-swirlds regarding the Notes for reviewer section: should we create issues for those two items now or they are not planned to be part of the initial modularization release? cc @netopyr

The ScheduleSign throttling needs to be done to maintain network limits.
I'm not sure about the "timed" schedule throttling; that might be worth bringing up in the modularization call.

Copy link
Member

@jsync-swirlds jsync-swirlds left a comment

Choose a reason for hiding this comment

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

A few clarifying questions.

Signed-off-by: Miroslav Gatsanoga <miroslav.gatsanoga@limechain.tech>
Signed-off-by: Miroslav Gatsanoga <miroslav.gatsanoga@limechain.tech>
Signed-off-by: Miroslav Gatsanoga <miroslav.gatsanoga@limechain.tech>
Copy link

codecov bot commented Nov 21, 2023

Codecov Report

All modified and coverable lines are covered by tests ✅

❗ No coverage uploaded for pull request base (develop@fd86222). Click here to learn what that means.

Additional details and impacted files
@@            Coverage Diff             @@
##             develop    #9994   +/-   ##
==========================================
  Coverage           ?   65.36%           
  Complexity         ?    30268           
==========================================
  Files              ?     3329           
  Lines              ?   127034           
  Branches           ?    13221           
==========================================
  Hits               ?    83039           
  Misses             ?    40755           
  Partials           ?     3240           

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

@MiroslavGatsanoga
Copy link
Collaborator Author

The ScheduleSign throttling needs to be done to maintain network limits.

Ok, I will add that to this PR as well then.

Signed-off-by: Miroslav Gatsanoga <miroslav.gatsanoga@limechain.tech>
Signed-off-by: Miroslav Gatsanoga <miroslav.gatsanoga@limechain.tech>
Signed-off-by: Miroslav Gatsanoga <miroslav.gatsanoga@limechain.tech>
Copy link

Node: HAPI Test (Time Consuming) Results

21 tests     4 ✔️  6m 13s ⏱️
  2 suites  17 💤
  2 files      0

Results for commit 6facfa6.

Copy link

Node: HAPI Test (Crypto) Results

211 tests   201 ✔️  19m 48s ⏱️
  22 suites    10 💤
  22 files        0

Results for commit 6facfa6.

Copy link

Node: HAPI Test (Token) Results

190 tests   188 ✔️  21m 32s ⏱️
  13 suites      2 💤
  13 files        0

Results for commit 6facfa6.

Copy link

Node: HAPI Test (Misc) Results

418 tests   273 ✔️  23m 6s ⏱️
  73 suites  145 💤
  73 files        0

Results for commit 6facfa6.

Copy link

Node: HAPI Test (Smart Contract) Results

410 tests   318 ✔️  42m 35s ⏱️
  56 suites    92 💤
  56 files        0

Results for commit 6facfa6.

@povolev15 povolev15 self-requested a review November 27, 2023 19:58
Copy link
Collaborator

@tinker-michaelj tinker-michaelj left a comment

Choose a reason for hiding this comment

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

All makes sense to me, tyvm @MiroslavGatsanoga !

@povolev15 povolev15 merged commit e5a516c into develop Nov 27, 2023
24 of 25 checks passed
@povolev15 povolev15 deleted the 09798-schedule-create-throttle branch November 27, 2023 20:24
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.

Fix Throttles to also account for the inner transaction in ScheduleCreate
6 participants