go-graphite / carbonzipper

proxy to transparently merge graphite carbon backends
Other
104 stars 29 forks source link

Initial tests for BroadcastGroup #61

Closed Civil closed 6 years ago

Civil commented 6 years ago

Initial implementation of tests for BroadcastGroup. More will follow.

Also fixes some bugs (empty requests, duplicate TLDs in list).

Also updates License to include Google LLC as co-owner (see with https://opensource.google.com/docs/patching/)

gunnihinn commented 6 years ago

Just FYI: The race condition unveiled by a53ec78 is in the logger. If we set the loglevel to 'info', then

$ while go test -race; do :; done

will run happily forever.

As far as I know, there is another race condition somewhere in the merge logic that we see in production and is not exhibited by these tests. I'm going to see if I can't get a reproducible test case from the prod data.

Civil commented 6 years ago

I haven't investigated it deeply, but I think it have the same root cause.

Write at 0x00c420190b48 by goroutine 45:
  github.com/go-graphite/carbonzipper/zipper/types.(*ServerFetchResponse).Merge()
      /home/civil/go/src/github.com/go-graphite/carbonzipper/zipper/errors/errors.go:92 +0x98c
  github.com/go-graphite/carbonzipper/zipper/broadcast.(*BroadcastGroup).Fetch()
      /home/civil/go/src/github.com/go-graphite/carbonzipper/zipper/broadcast/broadcast_group.go:237 +0xcd2
  github.com/go-graphite/carbonzipper/zipper/broadcast.TestFetchRequests.func1()
      /home/civil/go/src/github.com/go-graphite/carbonzipper/zipper/broadcast/broadcast_group_test.go:1174 +0xb3
{"level":"DEBUG","timestamp":"2018-05-08T21:18:39.004+0200","logger":"test","message":"sending request","type":"broadcastGroup","groupName":"many clients, different data","type":"fetch","request":["foo*"],"client_name":"client12"}
  testing.tRunner()
      /usr/lib/go/src/testing/testing.go:777 +0x16d

Previous read at 0x00c420190b48 by goroutine 51:
  reflect.Value.IsNil()
      /usr/lib/go/src/reflect/value.go:1016 +0xc8
  encoding/json.(*sliceEncoder).encode()
      /usr/lib/go/src/encoding/json/encode.go:731 +0x50
  encoding/json.(*sliceEncoder).(encoding/json.encode)-fm()
      /usr/lib/go/src/encoding/json/encode.go:747 +0x7b
  encoding/json.(*structEncoder).encode()
      /usr/lib/go/src/encoding/json/encode.go:639 +0x2dc
  encoding/json.(*structEncoder).(encoding/json.encode)-fm()
      /usr/lib/go/src/encoding/json/encode.go:653 +0x7b
  encoding/json.(*ptrEncoder).encode()
      /usr/lib/go/src/encoding/json/encode.go:780 +0x127
  encoding/json.(*ptrEncoder).(encoding/json.encode)-fm()
      /usr/lib/go/src/encoding/json/encode.go:785 +0x7b
  encoding/json.(*structEncoder).encode()
      /usr/lib/go/src/encoding/json/encode.go:639 +0x2dc
  encoding/json.(*structEncoder).(encoding/json.encode)-fm()
      /usr/lib/go/src/encoding/json/encode.go:653 +0x7b
  encoding/json.(*ptrEncoder).encode()
      /usr/lib/go/src/encoding/json/encode.go:780 +0x127
  encoding/json.(*ptrEncoder).(encoding/json.encode)-fm()
      /usr/lib/go/src/encoding/json/encode.go:785 +0x7b
  encoding/json.(*encodeState).reflectValue()
      /usr/lib/go/src/encoding/json/encode.go:325 +0x93
 encoding/json.(*encodeState).marshal()
      /usr/lib/go/src/encoding/json/encode.go:298 +0xb2
  encoding/json.Marshal()
      /usr/lib/go/src/encoding/json/encode.go:161 +0xb9
  github.com/go-graphite/carbonzipper/vendor/go.uber.org/zap/zapcore.(*jsonEncoder).AddReflected()
      /home/civil/go/src/github.com/go-graphite/carbonzipper/vendor/go.uber.org/zap/zapcore/json_encoder.go:128 +0x5d
  github.com/go-graphite/carbonzipper/vendor/go.uber.org/zap/zapcore.Field.AddTo()
      /home/civil/go/src/github.com/go-graphite/carbonzipper/vendor/go.uber.org/zap/zapcore/field.go:159 +0xb72
  github.com/go-graphite/carbonzipper/vendor/go.uber.org/zap/zapcore.addFields()
      /home/civil/go/src/github.com/go-graphite/carbonzipper/vendor/go.uber.org/zap/zapcore/field.go:199 +0x91
  github.com/go-graphite/carbonzipper/vendor/go.uber.org/zap/zapcore.(*jsonEncoder).EncodeEntry()
      /home/civil/go/src/github.com/go-graphite/carbonzipper/vendor/go.uber.org/zap/zapcore/json_encoder.go:339 +0x29e
  github.com/go-graphite/carbonzipper/vendor/go.uber.org/zap/zapcore.(*ioCore).Write()
      /home/civil/go/src/github.com/go-graphite/carbonzipper/vendor/go.uber.org/zap/zapcore/core.go:86 +0xce
  github.com/go-graphite/carbonzipper/vendor/go.uber.org/zap/zapcore.(*CheckedEntry).Write()
      /home/civil/go/src/github.com/go-graphite/carbonzipper/vendor/go.uber.org/zap/zapcore/entry.go:215 +0x14c
  github.com/go-graphite/carbonzipper/vendor/go.uber.org/zap.(*Logger).Debug()
      /home/civil/go/src/github.com/go-graphite/carbonzipper/vendor/go.uber.org/zap/logger.go:179 +0x95
  github.com/go-graphite/carbonzipper/zipper/broadcast.(*BroadcastGroup).Find()
      /home/civil/go/src/github.com/go-graphite/carbonzipper/zipper/broadcast/broadcast_group.go:317 +0x1f4d
  github.com/go-graphite/carbonzipper/zipper/broadcast.(*BroadcastGroup).doSingleFetch()
      /home/civil/go/src/github.com/go-graphite/carbonzipper/zipper/broadcast/broadcast_group.go:141 +0xcf4

which if I'm correct, means that it's writing to the same variable while reading it, this is not a problem in zap, but race condition inside zipper.

Civil commented 6 years ago

I've merged this branch, cause anyway it's mostly test-related and license-related. But at least there is some tests in master.