-
Notifications
You must be signed in to change notification settings - Fork 232
Define json models for SamplingStrategyResponse and use them #103
Conversation
- Use our own json models instead of depending on thrift generated classes. This will help up when we move to manual thrift generation - Use Google's gson instead of jackson-databind - Add tests for HttpSamplingManager
*/ | ||
package com.uber.jaeger.samplers.http; | ||
|
||
import lombok.Value; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@value automatically sets the class to final, and fields to be private final
See https://projectlombok.org/features/Value.html
2 similar comments
2 similar comments
LGTM although I'm not a fan of "samplers/http/" as a directory name for json models. |
@@ -16,6 +16,7 @@ ext.jerseyVersion = '2.22.2' | |||
ext.slf4jVersion = '1.7.16' | |||
ext.jacksonVersion = '2.7.4' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
do we still need this then?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it's used by crossdock
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if only by crossdock, let's move it there, so it's less confusing at the top level.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, it seems crossdock has it's own version. It's also in use by jaeger-jaxrs2, but it seems that it's unused.
I deleted it.
1 similar comment
LGTM |
This will help up when we move to manual thrift generation