IS-450 Reload rotation timestamp on each isExitTime request #1757
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
SUGGESTIONS BEFORE MERGE:
|
badrogger
requested review from
DmytroNazarenko and
kladkogex
as code owners
DmytroNazarenko
requested changes
Dec 18, 2023
| } | ||
|
|
||
| bool InstanceMonitor::isTimeToRotate( uint64_t _finishTimestamp ) { | ||
| if ( !fs::exists( m_rotationInfoFilePath ) ) { | ||
| return false; | ||
| } | ||
| return m_finishTimestamp <= _finishTimestamp; |
There was a problem hiding this comment.
It's better to write information about rotation into logs
libethereum/InstanceMonitor.cpp
Outdated
| @@ -46,23 +46,20 @@ void InstanceMonitor::initRotationParams( uint64_t _finishTimestamp ) { | |||
| std::ofstream rotationInfoFile( m_rotationInfoFilePath.string() ); | |||
| rotationInfoFile << rotationJson; | |||
|
|
|||
| m_finishTimestamp = _finishTimestamp; | |||
| LOG( m_logger ) << "Set rotation time to " << m_finishTimestamp; | |||
| LOG( m_logger ) << "Set rotation time to " << _finishTimestamp; | |||
| } | |||
|
|
|||
| bool InstanceMonitor::isTimeToRotate( uint64_t _finishTimestamp ) { | |||
There was a problem hiding this comment.
Better to rename the parameter to currentTimestamp/blockTimestamp or sth like this
kladkogex
reviewed
Dec 27, 2023
libethereum/InstanceMonitor.cpp
Outdated
| uint64_t InstanceMonitor::rotationTimestamp() const { | ||
| std::ifstream rotationInfoFile( m_rotationInfoFilePath.string() ); | ||
| auto rotationJson = nlohmann::json::parse( rotationInfoFile ); | ||
| auto timestamp = rotationJson["timestamp"].get< uint64_t >(); |
There was a problem hiding this comment.
Add try catch to handle the case of corrupt file
kladkogex
reviewed
Dec 27, 2023
libethereum/InstanceMonitor.cpp
Outdated
| @@ -46,23 +46,22 @@ void InstanceMonitor::initRotationParams( uint64_t _finishTimestamp ) { | |||
| std::ofstream rotationInfoFile( m_rotationInfoFilePath.string() ); | |||
| rotationInfoFile << rotationJson; | |||
|
|
|||
| m_finishTimestamp = _finishTimestamp; | |||
| LOG( m_logger ) << "Set rotation time to " << m_finishTimestamp; | |||
| LOG( m_logger ) << "Set rotation time to " << _finishTimestamp; | |||
kladkogex
reviewed
Dec 27, 2023
libethereum/InstanceMonitor.h
Outdated
| @@ -61,5 +56,5 @@ class InstanceMonitor { | |||
| void reportExitTimeReached( bool _reached ); | |||
|
|
|||
| private: | |||
| dev::Logger m_logger{ createLogger( dev::VerbosityInfo, "instance-monitor" ) }; | |||
| mutable dev::Logger m_logger{ createLogger( dev::VerbosityInfo, "instance-monitor" ) }; | |||
There was a problem hiding this comment.
mutable is not needed here
dimalit
previously approved these changes
Dec 28, 2023
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## v3.18.0 #1757 +/- ##
===========================================
+ Coverage 45.52% 45.61% +0.08%
===========================================
Files 356 356
Lines 51709 51727 +18
===========================================
+ Hits 23542 23593 +51
+ Misses 28167 28134 -33 |
kladkogex
previously approved these changes
Jan 4, 2024
badrogger
changed the title
dimalit
previously approved these changes
Jan 4, 2024
DmytroNazarenko
requested changes
Jan 4, 2024
libethereum/InstanceMonitor.h
Outdated
Comment on lines
73
to
74
| mutable dev::Logger m_info_logger{ createLogger( dev::VerbosityInfo, "instance-monitor" ) }; | ||
| mutable dev::Logger m_error_logger{ createLogger( dev::VerbosityError, "instance-monitor" ) }; |
There was a problem hiding this comment.
Change to camelCase
DmytroNazarenko
approved these changes
Jan 5, 2024
olehnikolaiev
approved these changes
Jan 8, 2024
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Problem
The solution for https://github.com/skalenetwork/internal-support/issues/450 making skale-admin will write it to corresponding file directly instead of sending rpc call. The issue is that InstanceMonitor.cpp loads this file once during startup, which effectively says that timestamp won't be passed to skaled at all.
Changes
isExitTimewill check the content of rotation.txt file and compare it to current block timestamp instead on relaying onm_finishTimestampfield.Performance
Since
rotation.txtfile is only 4 bytes and reading is happening in background performance should not be affected.Unit tests
Modified tests in
InstanceMonitorTest.cpp.Testing
Tested by running rotation procedure on local network.