Add distribution support to FastForward client #13
Conversation
| @@ -18,24 +18,6 @@ | |||
| * -/-/- | |||
| */ | |||
|
|
|||
| /* | |||
| * FastForward Client | |||
There was a problem hiding this comment.
I believe this needs to be at the top of all our open source files, would be good to add back
There was a problem hiding this comment.
Not sure what's happening, but if you check the actual file you will see the header.
| } | ||
|
|
||
| @Test | ||
| public void testSerializeDisTributionValue() throws InvalidProtocolBufferException { |
| public void send(Metric metric) throws IOException { | ||
| sendFrame(metric.serialize()); |
There was a problem hiding this comment.
should both of these send functions just be combined into one that takes in the version?
There was a problem hiding this comment.
Are you thinking about something like this send(Metric metric , Version v1) ?if not, Give me the signature of the combine function you have in mind.
There was a problem hiding this comment.
I guess I'm a little confused by both send functions having the same name but maybe that's how it needs to be written and I'm missing something. Was thinking something like public void send(Metric metric, version) but since it's two different types of metrics, maybe we can just be explicit with the names if possible?
public void sendV0(Metric metric) throws IOException {
sendFrame(metric.serialize(), Version.V0);
}
public void sendV1(com.spotify.ffwd.v1.Metric metric) throws IOException {
sendFrame(metric.serialize(), Version.V1);
}
There was a problem hiding this comment.
Ok, I see your point now. We are not using a different name because we don't want to change ForwardClient public interface. With different names such as sendV1 or sendV0, users will have to make a code change whether they are using distribution or not. This is something we want to avoid.
With the overload approach (using the same name), you don't have to change anything if you are not using distribution.
There was a problem hiding this comment.
Okay cool, thank you for clarifying :)
There was a problem hiding this comment.
Great! can you merge it for me.
There was a problem hiding this comment.
You should be able to merge now :)
| builder.setValue(Protocol1.Value.newBuilder().setDistributionValue(byteString)); | ||
| } else { | ||
| throw new IllegalArgumentException("Failed to identify distribution type" + value); |
There was a problem hiding this comment.
nit pick - space needed after word "type"
| @@ -80,7 +62,7 @@ public Metric( | |||
| this.has = has; | |||
There was a problem hiding this comment.
It is probably safe to keep it for now.
In the context of Metric class, it is used to determine which attribute is set.
| builder.setValue(Protocol1.Value.newBuilder().setDistributionValue(byteString)); | ||
| } else { | ||
| throw new IllegalArgumentException("Failed to identify distribution type : [" + value |
There was a problem hiding this comment.
Why does the error message say "distribution type" instead of "value type"?
This change overload FastForward client send method.
Why do we need this change ?
This change is needed to add distribution support to Semantic Metric.
Are heroic users affected by this change ?
No, Heroic users won't be affected by this change.