Fix underflow in performance calculation #1035
Merged
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
KtorZ
reviewed
Nov 14, 2019
lib/core/src/Cardano/Pool/Metrics.hs
Outdated
| toInt (EpochNo e) = fromIntegral e | ||
|
|
||
| toEp :: Int -> EpochNo | ||
| toEp = EpochNo . fromIntegral |
There was a problem hiding this comment.
Alternatively:
historicalEpochs :: [EpochNo]
historyEpochs
| currentEpoch < 14 -> [0..currentEpoch]
| otherwise -> [currentEpoch - 14 .. currentEpoch]?
There was a problem hiding this comment.
Neat,
although maybe it is more difficult to see that it is indeed correct
maybe
historicalEpochs :: [EpochNo]
historicalEpochs
| currentEpoch < window = [0..currentEpoch]
| otherwise = [currentEpoch - window .. currentEpoch]
where
window = 14 historicalEpochs :: [EpochNo]
historicalEpochs
| currentEpoch > window = [currentEpoch - window .. currentEpoch]
| otherwise = [0..currentEpoch]
where
window = 14but it still kind of relies on you knowing that currenEpoch and window are non-negative… but the same can probably be argued for the original min solution.
There was a problem hiding this comment.
Hmmm. I am not sure to get where you are going to with this? We know currentEpoch is non-negative because it's an EpochNo.
NOTE: Does not catch the underflow bug as the epochs are too high in the integration tests.
Anviking
force-pushed
the
anviking/fix-underflow
branch
from
c4dc29e
to
a1fe6d1
Compare
KtorZ
approved these changes
Nov 14, 2019
|
bors r+ |
iohk-bors bot
added a commit
that referenced
this pull request
Nov 14, 2019
1031: Test that follow doesn't retry too often r=KtorZ a=Anviking # Issue Number #1027 # Overview <!-- Detail in a few bullet points the work accomplished in this PR --> - [x] I have added tests to make sure we retry (and log), but not too much. # Comments - The test just hangs *without* the fix `git revert bf8844d` 🤔 I guess some infinite loops happens, but I can't figure out how to fix it. Not sure if we need to fix it, or want this PR anyway. <!-- Don't forget to: ✓ Self-review your changes to make sure nothing unexpected slipped through ✓ Assign yourself to the PR ✓ Assign one or several reviewer(s) ✓ Once created, link this PR to its corresponding ticket ✓ Acknowledge any changes required to the Wiki --> 1033: remove race condition from TRANS_DELETE_01 r=KtorZ a=paweljakubas # Issue Number <!-- Put here a reference to the issue this PR relates to and which requirements it tackles --> # Overview <!-- Detail in a few bullet points the work accomplished in this PR --> - [x] I have addressed issue of race condition in TRANS_DELETE_01 # Comments <!-- Additional comments or screenshots to attach if any --> <!-- Don't forget to: ✓ Self-review your changes to make sure nothing unexpected slipped through ✓ Assign yourself to the PR ✓ Assign one or several reviewer(s) ✓ Once created, link this PR to its corresponding ticket ✓ Acknowledge any changes required to the Wiki --> 1034: bump version to 2019-11-14 r=KtorZ a=KtorZ # Issue Number <!-- Put here a reference to the issue this PR relates to and which requirements it tackles --> N/A # Overview <!-- Detail in a few bullet points the work accomplished in this PR --> - [ ] I have bumped all version numbers to `2019.11.14` in prevision of the next release. # Comments <!-- Additional comments or screenshots to attach if any --> <!-- Don't forget to: ✓ Self-review your changes to make sure nothing unexpected slipped through ✓ Assign yourself to the PR ✓ Assign one or several reviewer(s) ✓ Once created, link this PR to its corresponding ticket ✓ Acknowledge any changes required to the Wiki --> 1035: Fix underflow in performance calculation r=KtorZ a=Anviking # Issue Number # Overview <!-- Detail in a few bullet points the work accomplished in this PR --> - [x] I have fixed underflow by converting from `EpochNo` to `Int` and back. - [x] I made integration tests stronger.⚠️ Does not catch the bug however, as the current epoch is too high. (needs to be 14 or lower to underflow) # Comments - Oops. <!-- Additional comments or screenshots to attach if any --> <!-- Don't forget to: ✓ Self-review your changes to make sure nothing unexpected slipped through ✓ Assign yourself to the PR ✓ Assign one or several reviewer(s) ✓ Once created, link this PR to its corresponding ticket ✓ Acknowledge any changes required to the Wiki --> Co-authored-by: Johannes Lund <johannes.lund@iohk.io> Co-authored-by: KtorZ <matthias.benkort@gmail.com> Co-authored-by: Pawel Jakubas <pawel.jakubas@iohk.io>
paweljakubas
approved these changes
Nov 14, 2019
Build succeeded |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Issue Number
Overview
EpochNotoIntand back.Comments