graphite-project / carbonate

Utilities for managing graphite clusters
MIT License
516 stars 80 forks source link

fill: fix bugs causing some data points not to be copied #90

Closed jehiah closed 6 years ago

jehiah commented 6 years ago

In the process of #89 I realized that the issue i was having wasn't the need for an overwrite flag, but rather just a missunderstanding that the behaviour i was seeing was due to bugs in fill where some data wasn't being copied between whisper data files to fill gaps.

The two bugs are

a) missing data points equal to secondsPerPoint were not filled (in my real-world use case, secondsPerPoint==86400 so data points gaps of a day are not repaired, but multi-data-points are b) trailing data gaps repaired (fill was not poperly called at the end of the loop)

I've updated test coverage to illustrate these bugs, and better codify expectations around fill with overwrite=False

cc: @deniszh @jssjr

jehiah commented 6 years ago

For context, the test updates against current code produce the following error

E       - [1, 2, 3, 4, 5, 6, 7, 8, 9, 10, 11, 12, 13, 14, 15, 16, 17, 18, 19, 20]
E       + [1.0,
E       +  2.0,
E       +  3.0,
E       +  4.0,
E       +  None,
E       +  6.0,
E       +  7.0,
E       +  8.0,
E       +  9.0,
E       +  10.0,
E       +  11.0,
E       +  12.0,
E       +  13.0,
E       +  14.0,
E       +  15.0,
E       +  16.0,
E       +  17.0,
E       +  18.0,
E       +  19.0,
E       +  None]
deniszh commented 6 years ago

LGTM. Please fix codacy issues - trailing whitespaces in lines 116 and 130. @jssjr ?

It's still not clear why travis do not installing carbon... Probably we should change tests to fix against master branch of carbon instead.

jehiah commented 6 years ago

@deniszh squashed in a whitespace fix.

deniszh commented 6 years ago

Merging