[bug] bucky-fill shouldn't cross-fill archives

[Civil]

The easiest way to reproduce the bug for me was:

1. Get some real wsp file with AggregationMethod=sum and multiple archives (original.wsp)
2. Make a copy of it with a new name (copy.wsp)
3. bucky-fill original.wsp copy.wsp

copy.wsp becomes corrupted - some points from Archive1 of original.wsp got written to Archive0 of copy.wsp

It seems that this happens with gaps in archive0 and archive1, but I'm not sure. I have an example real world data, but still trying to create a test-case out of that.

[Civil]

I've added a test case for this bug:

vsmirnov@ml905507 ~/go/gopath_third_party/src/github.com/jjneely/buckytools/fill $ go test -run=^TestTwo
2017/08/14 17:10:49 Running whisper-fill.py...
C       D       Python  Go      Simu
======  ======  ======  ======  ======
   6.0     6.0     3.0     3.0     6.0
   0.0     0.0     NaN     NaN     0.0
   9.0     9.0     6.0     6.0     9.0
   4.0     4.0     0.0     0.0     4.0
   3.0     3.0     9.0     9.0     3.0
   1.0     1.0     4.0    14.0     1.0
   2.0     2.0     3.0     3.0     2.0
   4.0     4.0     1.0     1.0     4.0
   4.0     4.0     2.0     2.0     4.0
   0.0     0.0     4.0     4.0     0.0
   4.0     4.0     4.0    21.0     4.0
   5.0     5.0     0.0     0.0     5.0
   8.0     8.0     4.0     4.0     8.0
   0.0     0.0     5.0     5.0     0.0
   1.0     1.0     8.0     8.0     1.0
   6.0     6.0     0.0    17.0     6.0
   5.0     5.0     1.0     1.0     5.0
   5.0     5.0     6.0     6.0     5.0
   0.0     0.0     5.0     5.0     0.0
   6.0     6.0     5.0     5.0     6.0
   8.0     8.0     0.0    27.0     8.0
   7.0     7.0     6.0     6.0     7.0
   6.0     6.0     8.0     8.0     6.0
   6.0     6.0     7.0     7.0     6.0
   7.0     7.0     6.0     6.0     7.0
   4.0     4.0     6.0    17.0     4.0
   0.0     0.0     7.0     7.0     0.0
   0.0     0.0     4.0     4.0     0.0
   2.0     2.0     0.0     0.0     2.0
   4.0     4.0     0.0     0.0     4.0

--- FAIL: TestTwoArchives (0.19s)
        fill_test.go:342: Whipser point 0 is NaN but should be 3.000000

        fill_test.go:349: Whipser point 0 is NaN but should be 3.000000

FAIL
exit status 1
FAIL    github.com/jjneely/buckytools/fill      0.199s

As you can see here in Go code there are points that are actually sum of the points between them, and that shouldn't happen

I'm also not sure why fetchFromFile and C is that much different.

[jjneely]

Thanks for this.

[Civil]

I'm still not sure how to write test that it would show normal data in first two columns, but I'll try to do that tomorrow (if you won't do that instead of me).

Though I also have an idea of how to fix that: extend whisper library with ability to update archive with certain retention period (e.x. generalize current function to accept 'retention int' as a parameter and if it's not 0, skip archives that doesn't match desired retention).

[Civil]

Ok, I've added a proper test for this particular issue, though I've hardcoded dataset for it.

Though fill still missing tests that will check archives other than 1st one.

[Civil]

I've also pushed my idea of fix for that issue to original PR.

[Civil]

@jjneely any news on this bug? My patch works really well in my tests, so can you please have a look at it and either merge it or suggest another solution for this problem?

[jjneely]

Looks sane, but I want to spend some time to review.

[azhiltsov]

@jjneely did you have any spare cycles for review by any chance?

[jjneely]

I have a upcoming project at work that will afford me some time here, however, more urgent matters keep emerging. Its on the list, I'm just low on manpower.

[jjneely]

Apologies, I want to take the corruption issues seriously, and this is a work sponsored project -- which you would think would provide ample time to work on it. But the goal posts keep moving.

[Civil]

Hi!

Please have a look at the PR to solve this problem.
Pros to merge it as-is even without an in-depth look:

1. There is a test case that fails with current code
2. After the fix it's not failing anymore
3. No other tests fails

Assuming that there are tests for most of stuff already in place, it should be safe to merge it as-is now and investigate problem in-depth later.

[Civil]

ping

[Civil]

@jjneely can you please have a look at this issue and at my PR?

I understand you might be short of time, but this issue cause corruption during backfill. I think this is very important to fix this, otherwise it makes backfill unusable (you will get garbage in backfilled files).

[towolf]

@jjneely seeing that you seem to have no more time for this project and @Civil seeming to be a competent submitter, can you please just merge this?

[Civil]

For now we decided to maintain our own fork with patches that we want to have in buckytools (see https://github.com/go-graphite/buckytools ). And we'll think about just rewriting buckytools from scratch to implement better support of our stack and to solve some other issues we have with it (e.x. that it requires to have centralized server and push data through it).

[gksinghjsr]

Can this pull request be merged, as it would help move the project forward? Also, can @Civil also be given admin rights?