postmates / cernan

telemetry aggregation and shipping, last up the ladder
Other
315 stars 11 forks source link

Avro source protocol v2 #451

Closed josephglanville closed 5 years ago

josephglanville commented 5 years ago

This introduces a new wire format version for the Avro source.

The existing version 1 Avro source doesn't provide any support for extensible metadata.

Introduced in version 2 is a set of key value pairs in the header that can be used to carry arbitrary metadata.

The protocol allows up to 255 key value pairs, each with a key of up to 255 UTF-8 encoded bytes and a value of up to 65535 bytes in length.

The new packet structure is described below in IETF RFC format:

 0                   1                   2                   3
 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
|                             Length                            |
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
|                            Version                            |
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
|                            Control                            |
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
|                                                               |
+                               ID                              +
|                                                               |
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
|                                                               |
+                            ShardBy                            +
|                                                               |
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
|   #KV Pairs   |   Key Length  |    Key (up to 255 bytes)      |
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
|          Value Length         |   Value (up to 65535 bytes)   |
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
|                          Avro N Bytes                         |
+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

The source remains backwards compatible with version 1 clients.

codecov[bot] commented 5 years ago

Codecov Report

Merging #451 into master will decrease coverage by 0.05%. The diff coverage is 91.96%.

@@            Coverage Diff             @@
##           master     #451      +/-   ##
==========================================
- Coverage      92%   91.94%   -0.06%     
==========================================
  Files          30       30              
  Lines        4201     4310     +109     
==========================================
+ Hits         3865     3963      +98     
- Misses        336      347      +11
pulltab commented 5 years ago

@blt Still labelled a WIP, but can we have your eyes on this changeset too?

blt commented 5 years ago

Hi folks! Happy to give this a review. Can I get a description added to the PR so I know what this is trying to accomplish?

josephglanville commented 5 years ago

@blt this is ready for initial review but is yet to undergo full end to end testing.

josephglanville commented 5 years ago

I type aliased util::HashMap<Vec<u8>, Vec<u8>> to Metadata which cleaned things up a bit. Though Default:: default () feels a little janky. Would be nice if Metadata::new() worked without additional code. Might research that a bit if I get time.

blt commented 5 years ago

@josephglanville you ought to be able to do Metadata::default(), no?

josephglanville commented 5 years ago

@blt fixed a bug in the header handling and improved test coverage to meet the targets.