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(cwhisper): Fixing buffer merging logic #35

Merged
merged 2 commits into from
Aug 11, 2022

Conversation

deniszh
Copy link
Member

@deniszh deniszh commented Aug 11, 2022

Cwhisper buffer is circular, so, fetchCompressed return unsorted data and then it should be sorted - we're creating new array named values, calculate index and copy data from results there. But I found out that sometimes if select interval (end - start) is smaller then buffer size than logic breaks.
E.g. if buffer has 120 points and we're selecting last 1 hour - it works:
Screenshot 2022-08-11 at 12 44 28
if 1h minus 5min - it breaks:
Screenshot 2022-08-11 at 12 44 11
So, in fix below I create values with same size as fetchCompressed data and then cut rest before return.

@deniszh deniszh changed the title cwhisper(fix): Fixing buffer merging logic fix(cwhisper): Fixing buffer merging logic Aug 11, 2022
@deniszh deniszh merged commit 7b04412 into go-graphite:master Aug 11, 2022
@deniszh deniszh deleted the dzhdanov/null5m-bugfix branch August 11, 2022 11:35
@bom-d-van
Copy link
Member

Hi @deniszh , just in case, this seems to have failed one of the test case. (ci isn't enabled in go-whisper, so it wasn't reported here).

--- FAIL: TestCompressedWhisperReadWrite1 (0.00s)
    --- FAIL: TestCompressedWhisperReadWrite1/buffer_overflow (0.00s)
        compress_test.go:295:   []whisper.TimeSeriesPoint{
              	{Time: 1660219749, Value: NaN},
              	{Time: 1660219750, Value: NaN},
              	{Time: 1660219751, Value: NaN},
            + 	{Time: 1660219752, Value: NaN},
            + 	{Time: 1660219753, Value: 10},
            + 	{Time: 1660219754, Value: NaN},
            + 	{Time: 1660219755, Value: NaN},
            + 	{Time: 1660219756, Value: NaN},
            + 	{Time: 1660219757, Value: NaN},
            + 	{Time: 1660219758, Value: 14},
            + 	{Time: 1660219759, Value: NaN},
            + 	{Time: 1660219760, Value: NaN},
            + 	{Time: 1660219761, Value: NaN},
            + 	{Time: 1660219762, Value: NaN},
            + 	{Time: 1660219763, Value: 15},
              }

--- FAIL: TestCompressedWhisperReadWrite2 (0.00s)
    compress_test.go:368:   &whisper.TimeSeries{
          	fromTime:  1544477925,
          	untilTime: 1544477945,
          	step:      5,
          	values: []float64{
        - 		NaN,
        + 		NaN, 666, NaN, NaN,
          	},
          }

    compress_test.go:389:   &whisper.TimeSeries{
          	fromTime:  1544478201,
          	untilTime: 1544478231,
          	step:      1,
          	values: []float64{
          		... // 10 identical elements
          		24,
          		15,
        + 		1,
        + 		2,
        + 		NaN,
        + 		NaN,
        + 		NaN,
        + 		NaN,
        + 		NaN,
        + 		NaN,
        + 		NaN,
        + 		NaN,
        + 		NaN,
        + 		3,
        + 		4,
        + 		15.5,
        + 		14.0625,
        + 		3.25,
        + 		8.625,
        + 		13.1,
          	},
          }

--- FAIL: TestCompressedWhisperReadWrite3 (0.54s)
    --- FAIL: TestCompressedWhisperReadWrite3/random_time (0.54s)
        compress_test.go:495: case: random_time
        compress_test.go:622: statTotalUpdates: 420 extended: 2 totalPoints: 30774
        compress_test.go:647: go run cmd/compare.go -v -now 1589720099 tmp/test3_random_time.wsp tmp/test3_random_time.wsp.cwsp
panic: runtime error: index out of range [86] with length 86 [recovered]
	panic: runtime error: index out of range [86] with length 86

goroutine 37 [running]:
testing.tRunner.func1.2({0x11d4560, 0xc00001e198})
	/usr/local/go/src/testing/testing.go:1209 +0x24e
testing.tRunner.func1()
	/usr/local/go/src/testing/testing.go:1212 +0x218
panic({0x11d4560, 0xc00001e198})
	/usr/local/go/src/runtime/panic.go:1038 +0x215
github.com/go-graphite/go-whisper.Compare({0xc000018d80, 0x19}, {0xc000500940, 0x1e}, 0x5ec13423, 0x0, {0x0, 0x1052300}, 0x0, 0x0, ...)
	/Users/bom_d_van/Code/go/workspace/src/github.com/go-graphite/go-whisper/debug.go:400 +0x1494
github.com/go-graphite/go-whisper.TestCompressedWhisperReadWrite3.func18(0xc000165ba0)
	/Users/bom_d_van/Code/go/workspace/src/github.com/go-graphite/go-whisper/compress_test.go:649 +0xf8d
testing.tRunner(0xc000165ba0, 0xc0001b6780)
	/usr/local/go/src/testing/testing.go:1259 +0x102
created by testing.(*T).Run
	/usr/local/go/src/testing/testing.go:1306 +0x35a
exit status 2
FAIL	github.com/go-graphite/go-whisper	0.877s

@deniszh
Copy link
Member Author

deniszh commented Aug 11, 2022

Oh, you're right, forgot about tests :D
Doesn't looks good, let me check.

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.

None yet

2 participants