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

Fix multiple blocks with the same block start returned from fetchblocks pathway #1707

Merged
merged 11 commits into from
Jun 7, 2019

Conversation

richardartoul
Copy link
Contributor

@richardartoul richardartoul commented Jun 6, 2019

Fixes #1640

@martin-mao This is the bug we were talking about last time I saw you.

// TODO(r): pool these results arrays
// Two-dimensional slice such that the first dimension is unique by blockstart
// and the second dimension is blocks of data for that blockstart (not necessarily
// in chronological order).
Copy link
Collaborator

Choose a reason for hiding this comment

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

Seems like a copy pasta comment from above that doesn't follow here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I refactored it to be more clear, take another look

// in chronological order).
//
// ex. (querying 2P.M -> 6P.M with a 2-hour blocksize):
// [][]block.FetchBlockResult{
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is actually a 1D slice which is one block start per element.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refactored to be more clear, take another look

Copy link
Collaborator

@robskillington robskillington left a comment

Choose a reason for hiding this comment

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

LGTM

@codecov
Copy link

codecov bot commented Jun 6, 2019

Codecov Report

Merging #1707 into master will decrease coverage by 27.9%.
The diff coverage is 5%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #1707      +/-   ##
=========================================
- Coverage    71.9%   43.9%     -28%     
=========================================
  Files         970     944      -26     
  Lines       81427   79516    -1911     
=========================================
- Hits        58551   34961   -23590     
- Misses      19037   41330   +22293     
+ Partials     3839    3225     -614
Flag Coverage Δ
#aggregator 61.2% <ø> (-21.3%) ⬇️
#cluster 30% <ø> (-55.7%) ⬇️
#collector 39.2% <ø> (-24.7%) ⬇️
#dbnode 58.1% <5%> (-21.9%) ⬇️
#m3em 44.2% <ø> (-29%) ⬇️
#m3ninx 52.4% <ø> (-21.7%) ⬇️
#m3nsch 100% <ø> (+48.8%) ⬆️
#metrics 17.6% <ø> (ø) ⬆️
#msg 74.7% <ø> (ø) ⬆️
#query 16.7% <ø> (-49.7%) ⬇️
#x 47.1% <ø> (-38.7%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e249c30...f2b9c65. Read the comment docs.

1 similar comment
@codecov
Copy link

codecov bot commented Jun 6, 2019

Codecov Report

Merging #1707 into master will decrease coverage by 27.9%.
The diff coverage is 5%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master   #1707      +/-   ##
=========================================
- Coverage    71.9%   43.9%     -28%     
=========================================
  Files         970     944      -26     
  Lines       81427   79516    -1911     
=========================================
- Hits        58551   34961   -23590     
- Misses      19037   41330   +22293     
+ Partials     3839    3225     -614
Flag Coverage Δ
#aggregator 61.2% <ø> (-21.3%) ⬇️
#cluster 30% <ø> (-55.7%) ⬇️
#collector 39.2% <ø> (-24.7%) ⬇️
#dbnode 58.1% <5%> (-21.9%) ⬇️
#m3em 44.2% <ø> (-29%) ⬇️
#m3ninx 52.4% <ø> (-21.7%) ⬇️
#m3nsch 100% <ø> (+48.8%) ⬆️
#metrics 17.6% <ø> (ø) ⬆️
#msg 74.7% <ø> (ø) ⬆️
#query 16.7% <ø> (-49.7%) ⬇️
#x 47.1% <ø> (-38.7%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e249c30...f2b9c65. Read the comment docs.

@codecov
Copy link

codecov bot commented Jun 6, 2019

Codecov Report

Merging #1707 into master will decrease coverage by 6.5%.
The diff coverage is 97.5%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #1707     +/-   ##
========================================
- Coverage    71.9%   65.4%   -6.6%     
========================================
  Files         976     972      -4     
  Lines       81530   81350    -180     
========================================
- Hits        58693   53253   -5440     
- Misses      18996   24346   +5350     
+ Partials     3841    3751     -90
Flag Coverage Δ
#aggregator 78.9% <ø> (-3.6%) ⬇️
#cluster 74.4% <ø> (-11.4%) ⬇️
#collector 63.9% <ø> (ø) ⬆️
#dbnode 76% <97.5%> (-4.2%) ⬇️
#m3em 68.4% <ø> (-4.9%) ⬇️
#m3ninx 71.1% <ø> (-3.1%) ⬇️
#m3nsch 51.1% <ø> (ø) ⬆️
#metrics 17.6% <ø> (ø) ⬆️
#msg 74.7% <ø> (-0.2%) ⬇️
#query 51.4% <ø> (-15%) ⬇️
#x 75.5% <ø> (-10%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 294ce46...110d147. Read the comment docs.

@richardartoul richardartoul merged commit cef99ce into master Jun 7, 2019
@richardartoul richardartoul deleted the ra/fetch-blocks-fix branch June 7, 2019 14:16
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.

M3DB client should not reject multiple blocks from a peer with the same blockstart
3 participants