First draft of FFWD java client Metric with distribution support #10
Conversation
|
|
||
| public class Metric { | ||
|
|
||
| private static final long PROC = 1 << 0; |
There was a problem hiding this comment.
I believe this field is obsolete and we should clean and remove this from the list of fields if we introducing new schema.
ffwd-client/pom.xml
Outdated
| <dependency> | ||
| <groupId>com.google.protobuf</groupId> | ||
| <artifactId>protobuf-java</artifactId> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>com.google.guava</groupId> |
There was a problem hiding this comment.
Could introduction of guava dependency here create some issues downstream? I don't for sure if this could be an issue but all those java dependency hell issues.
| */ | ||
|
|
||
| /* | ||
| * FastForward Client |
There was a problem hiding this comment.
I think this should be removed as it repeated.
pom.xml
Outdated
| </properties> | ||
|
|
||
| <dependencyManagement> | ||
| <dependencies> | ||
| <dependency> | ||
| <groupId>com.google.guava</groupId> | ||
| <artifactId>guava</artifactId> |
| <version>${com.google.guava}</version> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>com.google.auto.value</groupId> |
There was a problem hiding this comment.
Can we avoid using auto value and try keep this client lean?
There was a problem hiding this comment.
I think there is a value in using auto value we don't have to write those crucial boilerplate methods and the code is more readable.
| <artifactId>auto-value</artifactId> | ||
| </dependency> | ||
| <dependency> | ||
| <groupId>javax.annotation</groupId> |
There was a problem hiding this comment.
Yes, library for annotation, it adds annotation to generated code.
| @@ -14,6 +14,15 @@ | |||
| <description>FastForward client for Java.</description> | |||
|
|
|||
| <dependencies> | |||
| <dependency> | |||
| <groupId>com.google.auto.value</groupId> | |||
There was a problem hiding this comment.
Can we avoid using auto value and try keep this client lean?
ao2017
changed the title
| return DoubleValue.create(value); | ||
| } | ||
|
|
||
| public static Value distributionValue(ByteBuffer byteBuffer) { |
There was a problem hiding this comment.
I think this should take ByteString as input instead of ByteBuffer since ByteBuffer is not immutable
This is the first cut of Metric with distribution (new histogram ) support. New features includes: