-
Notifications
You must be signed in to change notification settings - Fork 6
Add distribution support to FastForward client #13
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -18,24 +18,6 @@ | |
| * -/-/- | ||
| */ | ||
|
|
||
| /* | ||
| * FastForward Client | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I believe this needs to be at the top of all our open source files, would be good to add back
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Not sure what's happening, but if you check the actual file you will see the header. |
||
| * -- | ||
| * Copyright (C) 2016 - 2019 Spotify AB | ||
| * -- | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
|
|
||
| package com.spotify.ffwd.v1; | ||
|
|
||
| import com.google.protobuf.ByteString; | ||
|
|
@@ -67,7 +49,7 @@ public Metric() { | |
| this.has = 0; | ||
| this.time = 0; | ||
| this.key = null; | ||
| this.value = null; | ||
| this.value = Value.doubleValue(0); | ||
| this.host = null; | ||
| this.tags = new ArrayList<>(); | ||
| this.attributes = new HashMap<>(); | ||
|
|
@@ -80,7 +62,7 @@ public Metric( | |
| this.has = has; | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It is probably safe to keep it for now. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. got it. I didn't see that. 👍 |
||
| this.time = time; | ||
| this.key = key; | ||
| this.value = value; | ||
| this.value = (value == null) ? Value.doubleValue(0) : value; | ||
| this.host = host; | ||
| this.tags = tags; | ||
| this.attributes = attributes; | ||
|
|
@@ -103,7 +85,8 @@ public Metric key(String key) { | |
| } | ||
|
|
||
| public Metric value(Value value) { | ||
| return new Metric(set(VALUE), time, key, value, host, tags, attributes); | ||
| Value newValue = (value == null) ? Value.doubleValue(0) : value; | ||
| return new Metric(set(VALUE), time, key, newValue, host, tags, attributes); | ||
| } | ||
|
|
||
| public Metric host(String host) { | ||
|
|
@@ -149,10 +132,12 @@ public byte[] serialize() { | |
| builder.setValue(Protocol1.Value.newBuilder().setDoubleValue(doubleValue.getValue())); | ||
| } else if (value instanceof Value.DistributionValue) { | ||
| Value.DistributionValue distributionValue = (Value.DistributionValue) value; | ||
| ByteString byteString = ByteString.copyFrom(distributionValue.getValue()); | ||
| ByteString byteString = ByteString.copyFrom(distributionValue.getValue().array()); | ||
malish8632 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| builder.setValue(Protocol1.Value.newBuilder().setDistributionValue(byteString)); | ||
| } else { | ||
| throw new IllegalArgumentException("Failed to identify distribution type : [" + value | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why does the error message say "distribution type" instead of "value type"? |
||
| + "]"); | ||
| } | ||
|
|
||
| } | ||
|
|
||
| if (test(HOST)) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,93 @@ | ||
| /*- | ||
| * -\-\- | ||
| * FastForward Java Client | ||
| * -- | ||
| * Copyright (C) 2016 - 2020 Spotify AB | ||
| * -- | ||
| * Licensed under the Apache License, Version 2.0 (the "License"); | ||
| * you may not use this file except in compliance with the License. | ||
| * You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| * -/-/- | ||
| */ | ||
|
|
||
| package com.spotify.ffwd; | ||
|
|
||
| import com.spotify.ffwd.v1.Metric; | ||
| import java.io.IOException; | ||
| import java.net.SocketException; | ||
| import java.net.UnknownHostException; | ||
| import org.junit.jupiter.api.BeforeEach; | ||
| import org.junit.jupiter.api.Test; | ||
| import org.junit.jupiter.api.extension.ExtendWith; | ||
| import org.mockito.ArgumentCaptor; | ||
| import org.mockito.Mockito; | ||
| import org.mockito.MockitoAnnotations; | ||
| import org.mockito.Spy; | ||
|
|
||
| import static org.junit.jupiter.api.Assertions.assertArrayEquals; | ||
| import static org.junit.jupiter.api.Assertions.assertEquals; | ||
| import static org.mockito.Mockito.times; | ||
|
|
||
|
|
||
| public class FastForwardTest { | ||
| private static final String KEY = "key"; | ||
|
|
||
|
|
||
| @Spy | ||
| private FastForward client; | ||
|
|
||
| private ArgumentCaptor<byte []> inputCaptor; | ||
| private ArgumentCaptor<FastForward.Version> versionCaptor; | ||
|
|
||
| @BeforeEach | ||
| public void before() throws IOException{ | ||
| MockitoAnnotations.initMocks(this); | ||
| inputCaptor = ArgumentCaptor.forClass(byte[].class); | ||
| versionCaptor = ArgumentCaptor.forClass(FastForward.Version.class); | ||
| Mockito.doNothing().when(client).sendFrame(inputCaptor.capture(), versionCaptor.capture()); | ||
| } | ||
|
|
||
| @Test | ||
| public void testSendV1() throws SocketException, UnknownHostException , IOException { | ||
| final Metric metricV1 = FastForward.metricV1(KEY); | ||
| client.send(metricV1); | ||
| Mockito.verify(client,times(1)).sendFrame(Mockito.any(byte[].class) , | ||
| Mockito.any(FastForward.Version.class)); | ||
| assertArrayEquals(metricV1.serialize(), inputCaptor.getValue()); | ||
| assertEquals(FastForward.Version.V1,versionCaptor.getValue()); | ||
| } | ||
|
|
||
| @Test | ||
| public void testSendV0() throws SocketException, UnknownHostException , IOException { | ||
| final com.spotify.ffwd.Metric metricV0 = FastForward.metric(KEY); | ||
| client.send(metricV0); | ||
| Mockito.verify(client,times(1)).sendFrame(Mockito.any(byte[].class) , | ||
| Mockito.any(FastForward.Version.class)); | ||
| assertArrayEquals(metricV0.serialize(), inputCaptor.getValue()); | ||
| assertEquals(FastForward.Version.V0,versionCaptor.getValue()); | ||
| } | ||
|
|
||
| @Test | ||
| public void testMetricV0(){ | ||
| com.spotify.ffwd.Metric metricV0 = FastForward.metric(KEY); | ||
| assertEquals(com.spotify.ffwd.Metric.class, metricV0.getClass()); | ||
| assertEquals(KEY,metricV0.getKey()); | ||
| } | ||
|
|
||
| @Test | ||
| public void testMetricV1(){ | ||
| Metric metricV1 = FastForward.metricV1(KEY); | ||
| assertEquals(Metric.class, metricV1.getClass()); | ||
| assertEquals(KEY,metricV1.getKey()); | ||
| } | ||
|
|
||
|
|
||
| } |
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.
should both of these send functions just be combined into one that takes in the version?
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.
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.
Uh oh!
There was an error while loading. Please reload this page.
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.
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?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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Okay cool, thank you for clarifying :)
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.
Great! can you merge it for me.
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.
I'll get you permissions 👍
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.
You should be able to merge now :)