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.

Sign up for GitHub

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

Jump to bottom

Less strict engine QoS timer #4411

Merged
merged 7 commits into from
Sep 21, 2022
Merged

Less strict engine QoS timer #4411

merged 7 commits into from
Sep 21, 2022

Conversation

daniellehrner
Copy link
Contributor

Signed-off-by: Daniel Lehrner daniel.lehrner@consensys.net

PR description

Resets engine QoS timer with every call to the engine API instead of only when ExchangeTransitionConfiguration is called. ExchangeTransitionConfiguration mismatch will only submit a debug log not a warning anymore.

Fixed Issue(s)

fixes #4404

Documentation

  • I thought about documentation and added the doc-change-required label to this PR if
    updates are required.

Changelog

@daniellehrner
reset engine QoS timer with every call to the engine API, ExchangeTra…
9181939 
…nsitionConfiguration mismatch will only submit a debug log not a warning anymore

Signed-off-by: Daniel Lehrner <daniel.lehrner@consensys.net>
@daniellehrner
added CHANGELOG.md entry
44cc65b 
Signed-off-by: Daniel Lehrner <daniel.lehrner@consensys.net>
@daniellehrner daniellehrner marked this pull request as ready for review September 19, 2022 14:39
fab-10
fab-10 approved these changes Sep 19, 2022
View reviewed changes
Copy link
Contributor

@fab-10 fab-10 left a comment

Choose a reason for hiding this comment

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

LGTM, just a question,
why have not you done the common engineCallListener.executionEngineCalled() call in the parent class? in that wat it could be inherited easily by future engine apis

@daniellehrner
Copy link
Contributor Author

LGTM, just a question, why have not you done the common engineCallListener.executionEngineCalled() call in the parent class? in that wat it could be inherited easily by future engine apis

@fab-10 To make it possible to unit test. There is no acceptance test to test it and not really any other good way except for unit tests. As this timer will most probably be removed in future hard forks, I thought it is a justified tradeoff.

garyschulte
garyschulte approved these changes Sep 19, 2022
View reviewed changes
Copy link
Contributor

@garyschulte garyschulte left a comment

Choose a reason for hiding this comment

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

Looks great. There is a non-blocking DRY opportunity, but otherwise 🚢

.../org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineNewPayloadTest.java
@@ -116,6 +121,7 @@ public void shouldReturnValid() {
assertThat(res.getLatestValidHash().get()).isEqualTo(mockHeader.getHash());
assertThat(res.getStatusAsString()).isEqualTo(VALID.name());
assertThat(res.getError()).isNull();
verify(engineCallListener, times(1)).executionEngineCalled();
Copy link
Contributor

Choose a reason for hiding this comment

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

👍

...besu/ethereum/api/jsonrpc/internal/methods/engine/EngineExchangeTransitionConfiguration.java
@@ -70,8 +59,7 @@ public String getName() {

@Override
public JsonRpcResponse syncResponse(final JsonRpcRequestContext requestContext) {
// update our QoS "last call time"
getQosTimer().resetTimer();
engineCallListener.executionEngineCalled();
Copy link
Contributor

Choose a reason for hiding this comment

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

Minor point, we can DRY these up by adding this call into ExecutionEngineJsonRpcMethod.response() method rather than having it each engine api endpoint.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason why I did not do that, was because of the unit tests. When I move it there, I am not sure if I can still test the correct behavior with the unit tests. Do you think it would still be possible?

Fabio had the same question:
#4411 (comment)

garyschulte
garyschulte requested changes Sep 19, 2022
View reviewed changes
Copy link
Contributor

@garyschulte garyschulte left a comment •
edited
Loading

Choose a reason for hiding this comment

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

The failing unit test for EngineQosTimerTest implies something outside the try block is throwing before the async can complete.

...va/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineQosTimerTest.java Outdated Show resolved Hide resolved
...va/org/hyperledger/besu/ethereum/api/jsonrpc/internal/methods/engine/EngineQosTimerTest.java Outdated Show resolved Hide resolved
@daniellehrner
fixed EngineQosTimerTest
72a4f56 
Signed-off-by: Daniel Lehrner <daniel.lehrner@consensys.net>
@garyschulte
Copy link
Contributor

⛴️

@daniellehrner
shouldWarnWhenNotCalledWithinTimeout: logTimeoutWarning can be called…
8e83c48 
… more than once if the verification get delayed

Signed-off-by: Daniel Lehrner <daniel.lehrner@consensys.net>
@daniellehrner daniellehrner enabled auto-merge (squash) September 19, 2022 22:00
garyschulte
garyschulte approved these changes Sep 20, 2022
View reviewed changes
Copy link
Contributor

@garyschulte garyschulte left a comment

Choose a reason for hiding this comment

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

🚢

garyschulte and others added 2 commits September 20, 2022 11:14
@garyschulte
Merge branch 'main' into less_strict_engine_qos_timer
a9fab27
@daniellehrner
make tests less strict to consider unreliable timings in resource con…
c9dc073 
…straint systems, like CI

Signed-off-by: Daniel Lehrner <daniel.lehrner@consensys.net>
@daniellehrner daniellehrner merged commit bc9ff86 into hyperledger:main Sep 21, 2022
@daniellehrner daniellehrner deleted the less_strict_engine_qos_timer branch September 21, 2022 16:31
eum602 pushed a commit to lacchain/besu that referenced this pull request Nov 3, 2023
@daniellehrner @eum602
Less strict engine QoS timer (hyperledger#4411)
fe907df 
* reset engine QoS timer with every call to the engine API, ExchangeTransitionConfiguration mismatch will only submit a debug log not a warning anymore

Signed-off-by: Daniel Lehrner <daniel.lehrner@consensys.net>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@fab-10 fab-10 fab-10 approved these changes

@garyschulte garyschulte garyschulte approved these changes

Assignees
No one assigned
Labels
mainnet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Reduce noise from Prysm & Nimbus warning messages
3 participants
@daniellehrner @garyschulte @fab-10