search

cisco-ie / pipeline-gnmi

A Model-Driven Telemetry collector based on the open-source tool pipeline
Apache License 2.0
32 stars 6 forks source link

Add Kafka 2.x Support #8

Closed remingtonc closed 5 years ago

remingtonc commented 5 years ago

This pull request adds Kafka 2.x support with minimal changes to the code. One breaking change is the requirement of the kafkaversion field in the kafka stage specification. This is due to Sarama requiring the Version to be specified, or otherwise defaulting to Kafka 0.8 interaction. This is a breaking config change but a good one for ongoing Kafka support. Thoughts?

There was also several errors during go vet which mentioned that some xport_gnmi.go code was unreachable. Specifically https://github.com/cisco-ie/pipeline-gnmi/blob/master/xport_gnmi.go#L315. It appears there is never an exit condition in the above for loop. I'm not totally certain - but I added a cancelled variable in order to have a clean exit. I think its placement in case <-ctx.Done(): would be correct. Do you agree @sbyx ?

Not ready for merge, requires validation and testing.

Resolves #3

sbyx commented 5 years ago

for ctx.Err() == nil {

should be shorter

BenderScript commented 5 years ago

Hello,

There are commits that do not seem related to Kafka 2.X support Example: .gitignore, go vet errors, build issues, etc. It would be better for the changes here to be directly related to the problem described on the issue. It makes for easier review, tracking, history, etc. We should open issues to fix unrelated problem.

Is a Datadog update needed for Kafka 2.x? Is it some kind of transitive dependency? I'm curious given Datadog is a stats cloud provide service.

thanks,

remingtonc commented 5 years ago

Updating sarama-related configs did not work out. Still receiving CRC errors even with kafkaversion set appropriately. Very problematic. Moving to pure sarama or confluent's library.

remingtonc commented 5 years ago

Was building off of master branch in production tests... :)

remingtonc commented 5 years ago

@repenno I agree with your assessment. Unfortunately I did the build process after the Kafka work - go build still compiled the repository, then I looked at the Makefile and that's where go vet and friends are run. Unfortunately it did result in a mixture of feature and housekeeping to properly build the feature. Do you think we should open separate branches, delete this one, and separate the work?

The DataDog dependency is a zstd implementation they released, a pretty nice compression algo released by Facebook last year or so. https://github.com/DataDog/zstd

BenderScript commented 5 years ago

@remingtonc I just hopped into the boat but traditionally the answer to your question depends on the release strategy you have envisioned. Do you intend to carry this back to master and release from there? Or are we going to release directly from a non-master branch (uncommon)? How are we tagging releases? From which branch are we creating and tagging customer specials?

It looks to me that some of your infrastructure commits (vets, build, fixing imports) could/should have gone into master directly in order to make it work and then pulled into any branch that needs it (Kafka being one of them).

remingtonc commented 5 years ago

Breaking into cleaner stages.