Add tests for XrayUDPStorage / Reporter.

[anuraaga]

I was randomly looking at the xray storage and want to convert to using Netty to likely improve performance a lot. But that would require benchmarking and reasoning about whether adding a dependency on Netty is OK (I guess so since other storage like elasticsearch has it).

In the meantime, it shouldn't require reasoning to use Netty for tests, which would help in the event we make a change to the storage implementation.

[codefromthecrypt]

main thing is this is used for a reporter which cant assume a netty dep. we could move the non netty code over there I suppose and split the two? https://github.com/openzipkin/zipkin-aws/blob/master/reporter-xray-udp/pom.xml#L35

…

On Fri, Mar 6, 2020, 6:23 PM Anuraag Agrawal ***@***.***> wrote: I was randomly looking at the xray storage and want to convert to using Netty to likely improve performance a lot. But that would require benchmarking and reasoning about whether adding a dependency on Netty is OK (I guess so since other storage like elasticsearch has it). In the meantime, it shouldn't require reasoning to use Netty for tests, which would help in the event we make a change to the storage implementation. ------------------------------ You can view, comment on, or merge this pull request online at: #157 Commit Summary - Add tests for XrayUDPStorage. File Changes - *M* pom.xml <https://github.com/openzipkin/zipkin-aws/pull/157/files#diff-600376dffeb79835ede4a0b285078036> (4) - *M* storage-xray-udp/pom.xml <https://github.com/openzipkin/zipkin-aws/pull/157/files#diff-06d30c5997098564f09d3e638c0c51da> (21) - *M* storage-xray-udp/src/test/java/zipkin2/storage/xray_udp/XRayUDPStorageTest.java <https://github.com/openzipkin/zipkin-aws/pull/157/files#diff-a37b9731692bf86d6de4f8d221db7261> (108) Patch Links: - https://github.com/openzipkin/zipkin-aws/pull/157.patch - https://github.com/openzipkin/zipkin-aws/pull/157.diff — You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub <#157?email_source=notifications&email_token=AAAPVVZVHKC5J7LJCGLR5I3RGDFJXA5CNFSM4LC5ERE2YY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4ITBZ4LA>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAPVV6KLTTB4TV42XSR56LRGDFJXANCNFSM4LC5EREQ> .

[anuraaga]

Yup did notice that and agree with splitting - it's a small amount of code to duplicate but with a good expectation for impact on the storage (non-reporter) side

[codefromthecrypt]

sgtm

…

On Fri, Mar 6, 2020, 6:28 PM Anuraag Agrawal ***@***.***> wrote: Yup did notice that and agree with splitting - it's a small amount of code to duplicate but with a good expectation for impact on the storage (non-reporter) side — You are receiving this because your review was requested. Reply to this email directly, view it on GitHub <#157?email_source=notifications&email_token=AAAPVVZFDPD7DMM64EGKVO3RGDF4HA5CNFSM4LC5ERE2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEOA3Z2A#issuecomment-595705064>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AAAPVVYAQ2DQ5HVPCDO3GFDRGDF4HANCNFSM4LC5EREQ> .

[anuraaga]

Added tests for reporter too in anticipation of the split.