graphite-compatible msgpack support #789
Conversation
|
do you want a review? |
| //msgp:ignore SeriesCompleterItem | ||
| //msgp:ignore SeriesTree | ||
| //msgp:ignore SeriesTreeItem | ||
|
|
There was a problem hiding this comment.
you can just add the generate line to the types you want, then you don't neede all these ignores
There was a problem hiding this comment.
Are you sure about that? https://github.com/tinylib/msgp/wiki/Using-the-Code-Generator
There was a problem hiding this comment.
you're right. in fact having the generate lines for types as we do now in some other places is misleading because it actually works on the file level.
| "github.com/tinylib/msgp/msgp" | ||
| ) | ||
|
|
||
| type Msgpack struct { |
There was a problem hiding this comment.
so it seems the whole and only reason we have this new type, and don't just keep using the old Msgp type for this new use case, is the slightly different header? otherwise they seem interchangeable. correct?
There was a problem hiding this comment.
Yes, that is the only difference.
| @@ -1,7 +1,7 @@ | |||
| { | |||
| "name":"graphite", | |||
| "type":"graphite", | |||
| "url":"http://graphite", | |||
| "url":"http://graphite:80", | |||
There was a problem hiding this comment.
I updated docker-dev to match docker-standard
There was a problem hiding this comment.
docker-dev has always worked fine for me without the explicit port, i recommend you take out this change out of the commit. it seems like setting the example to others in a confusing way (so we should probably simplify docker-standard, if anything)
There was a problem hiding this comment.
we specify the port for metrictank, I think it's clearer and more consistent to have it here also just so there's no confusion.
Makefile
Outdated
| @@ -13,6 +13,10 @@ bin-race: | |||
| ./scripts/build_tools.sh -race | |||
| docker: | |||
| ./scripts/build_docker.sh | |||
| dev: | |||
| ./scripts/build.sh | |||
| ./scripts/build_docker.sh | |||
There was a problem hiding this comment.
it is very rare that you have to rebuild the docker image when using the docker-dev stack. the whole point of the docker-dev stack is that it'll mount your custom binary and configs without having to build an image.
also you're better off using the launch.sh script (since it sets up a bunch of stuff up by default) instead of calling docker-compose directly.
I recommend taking this commit out of this PR, i would accept it as a new pr with the changes described
This adds support for
format=msgpackto/renderand/metrics/find