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

[large-tiles] Integrate cross block series reader #2543

Closed
wants to merge 148 commits into from

Conversation

linasm
Copy link
Collaborator

@linasm linasm commented Aug 19, 2020

What this PR does / why we need it:
Integrates CrossBlockReader into AggregateTiles process to allow aggregating series from multiple source blocks.

Special notes for your reviewer:

Does this PR introduce a user-facing and/or backwards incompatible change?:
NONE

Does this PR require updating code package or user-facing documentation?:
NONE

@codecov
Copy link

codecov bot commented Aug 19, 2020

Codecov Report

❗ No coverage uploaded for pull request base (large-tiles-aggregation@0dbbff1). Click here to learn what that means.
The diff coverage is n/a.

Impacted file tree graph

@@                    Coverage Diff                    @@
##             large-tiles-aggregation   #2543   +/-   ##
=========================================================
  Coverage                           ?   71.7%           
=========================================================
  Files                              ?    1083           
  Lines                              ?   95799           
  Branches                           ?       0           
=========================================================
  Hits                               ?   68696           
  Misses                             ?   22486           
  Partials                           ?    4617           
Flag Coverage Δ
#aggregator 76.0% <0.0%> (?)
#cluster 84.8% <0.0%> (?)
#collector 82.8% <0.0%> (?)
#dbnode 79.2% <0.0%> (?)
#m3em 74.4% <0.0%> (?)
#m3ninx 73.2% <0.0%> (?)
#m3nsch 51.1% <0.0%> (?)
#metrics 17.2% <0.0%> (?)
#msg 75.1% <0.0%> (?)
#query 67.6% <0.0%> (?)
#x 80.9% <0.0%> (?)

Flags with carried forward coverage won't be shown. Click here to find out more.


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 0dbbff1...dc8c65f. Read the comment docs.

@codecov
Copy link

codecov bot commented Aug 19, 2020

Codecov Report

Merging #2543 into large-tiles-aggregation will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@                   Coverage Diff                   @@
##           large-tiles-aggregation   #2543   +/-   ##
=======================================================
  Coverage                     48.8%   48.8%           
=======================================================
  Files                          948     948           
  Lines                        84952   84952           
=======================================================
  Hits                         41458   41458           
  Misses                       40076   40076           
  Partials                      3418    3418           
Flag Coverage Δ
#aggregator 76.0% <0.0%> (ø)
#cluster 62.9% <0.0%> (ø)
#collector 71.4% <0.0%> (ø)
#dbnode 58.7% <0.0%> (ø)
#m3em 51.3% <0.0%> (ø)
#m3ninx 46.7% <0.0%> (ø)
#m3nsch 100.0% <0.0%> (ø)
#query 14.7% <0.0%> (ø)
#x 42.3% <0.0%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


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 9916461...55d6dc2. Read the comment docs.

if !singleUnit {
// TODO: what happens if unit has changed mid-tile?
// Write early and then do the remaining values separately?
unit = frame.Units().Values()[downsampledValue.FrameIndex]
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will panic.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Tagging @arnikola (not sure if you were subscribed to this PR notifications; also, not sure what is the original branch for this piece of code). I believe @gediminasgu has fixed it in his downstream branch, but still IMHO we should do a fix at the source so that we don't run into this again.

if !singleAnnotation {
// TODO: what happens if annotation has changed mid-tile?
// Write early and then do the remaining values separately?
annotation = frame.Annotations().Values()[downsampledValue.FrameIndex]
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will panic too.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@arnikola (same as above).

@linasm linasm marked this pull request as draft September 8, 2020 18:20
Base automatically changed from large-tiles-aggregation to master September 14, 2020 19:11
@linasm
Copy link
Collaborator Author

linasm commented Sep 16, 2020

Closing as outdated.

@linasm linasm closed this Sep 16, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants