feat(new sink): Initial `pulsar` sink implementation #1665

leshow · 2020-01-31T20:51:55Z

closes #690

Missing tests and proper healthcheck right now, I just wanted to show this version to get some feedback. It's based largely on the kafka sink. It impl's Sink, like the kafka sink it holds the in flight futures and Acks back after completion.

Let me know if this approach is suitable, if there are some configuration options missing we'd like to add, etc. I'd like to hold off on SSL at the moment because I don't think it's well supported in the underlying crate. This change depends on a change to SinkContext and pulsar-rs also, worth looking at that.

binarylogic · 2020-01-31T22:00:13Z

@bruceg do you mind reviewing this?

binarylogic · 2020-01-31T22:40:22Z

And thanks for doing this @leshow.

bruceg

Given that it is mostly an adaptation of the kafka sink, it looks pretty straight forward. I presume at some point authentication support would be useful to add.

Without working healthcheck and tests I can't of course fully approve it, but what I see looks like a good start.

bruceg · 2020-02-01T00:41:31Z

src/sinks/pulsar.rs

+    let consumer = pulsar
+        .consumer()


I don't know in detail how Pulsar handles authorization, but trying to access the remote as a consumer in healthcheck but as a producer in the actual sink could hit different authorization paths.

I see the kafka sink also uses this approach, but it is a potential issue to be aware of.

bruceg · 2020-02-01T00:48:55Z

src/sinks/pulsar.rs

+                    }
+                    self.acker.ack(num_to_ack);
+                }
+                Err(e) => error!("future cancelled: {}", e),


This could use a better message. The kafka error at least referenced that it was a "delivery" future. The term "future" probably has little meaning to the average Vector user.

bruceg · 2020-02-01T00:58:06Z

It looks like a non-issue to me, but I'd feel more comfortable if @LucioFranco or @lukesteensen took a glance at the SinkContext change to make sure there wasn't some obvious problem with exposing the executor this way.

bruceg · 2020-02-01T01:00:47Z

BTW @leshow you can mark a pull request as "draft" when you submit it by clicking the arrow beside the "Create pull request".

leshow · 2020-02-01T01:20:56Z

Thanks for the feedback. Okay, will do for next time, I don't see an option to do it now. So long as the structure is okay I'll wrap up the healthcheck and tests hopefully in the next few days.

KannarFr · 2020-02-03T16:46:03Z

Hi, @leshow there is currently no authentication support on that PR, right?

leshow · 2020-02-03T17:40:33Z

@KannarFr Correct, however if all you need is the ability to pass the Authentication user/pass combo to pulsar I'm pretty sure I can add that in for you. https://docs.rs/pulsar/0.2.0/pulsar/struct.Pulsar.html#method.new

KannarFr · 2020-02-03T18:20:09Z

Pulsar is not using user/pass auth but I'm sure it's not difficult, I think I will commit that PR. :) Le lun. 3 févr. 2020 à 18:40, Evan Cameron <[email protected]> a écrit :

…

@KannarFr <https://github.com/KannarFr> Correct, however if all you need is the ability to pass the Authentication user/pass combo to pulsar I'm pretty sure I can add that in for you. https://docs.rs/pulsar/0.2.0/pulsar/struct.Pulsar.html#method.new — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#1665>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABZLQJOAMG3WKDL7SGYK72LRBBJJFANCNFSM4KONMSCA> .

KannarFr · 2020-02-03T23:08:57Z

Done in leshow#1.

LucioFranco

SinkContext changes look good 👍

leshow · 2020-02-06T20:35:29Z

I've added healthcheck & an integration test. Tried it locally with pulsar in the background and seems to work. I need to add another Consumer in the test still. This branch is also pointing at the master branch of pulsar-rs, which could be problematic.

binarylogic · 2020-02-06T20:55:12Z

@leshow sounds good. Just re-request a review when you're ready.

leshow · 2020-02-07T16:45:54Z

Okay, I've moved this out of WIP. It's working locally, if you want to reproduce, run pulsar-standalone in the background, and the integration test with:

cargo test pulsar --features pulsar-integration-test

I've added what I think are the relevant configs and toml files, but I may have missed something. I was just looking at other PRs and copying what they did.

edit: I'm not sure if you want to wait until pulsar-rs has a release before you actually merge this, I'll leave that up to you folks.

leshow · 2020-02-08T21:20:03Z

Seems like I still need to figure out the testing CI.

binarylogic · 2020-02-10T06:04:52Z

@leshow you can run them locally via Docker. See this README. Once you get the tests passing we'll review.

KannarFr · 2020-02-12T15:11:28Z

@leshow do you have time to do it? I can do it by my side if needed.

leshow · 2020-02-13T15:29:45Z

@KannarFr if you'd like to do the docker bit and submit it as a PR to my fork, sure. Other things have come up that have taken some time. You need to get pulsar-standalone running in a container so the integration tests can run.

Hoverbear · 2020-02-08T15:59:43Z

Cargo.toml

@@ -141,6 +141,7 @@ colored = "1.9"
 warp = "0.1.20"
 evmap = { version = "7", features = ["bytes"] }
 logfmt = "0.0.2"
+pulsar = { git = "https://github.com/wyyerd/pulsar-rs" }


At a minimum, we should be putting a checkout for this. (I realize the other git dependencies don't have one... Sorry for coming down on you.) Notably lockfiles aren't always used rust-lang/cargo#7169, and it would be nice to regain the ability to run cargo update or support cargo install.

Also, would we be comfortable waiting for a release of this library?

There is a big warning on https://github.com/wyyerd/pulsar-rs about future API changes:

Current status: Simple functionality, but expect API to change. Major API changes will come simultaneous with async-await stability, so look for that.

Absolutely, I mentioned elsewhere in this thread I'm not sure if you actually want to merge this until they do a release. In the meantime, I can add commit hash.

leshow · 2020-02-14T02:13:44Z

I'm at a bit of a loss as to how to add pulsar to the container in order to get the tests I added to run. I've also had an issue on at least 2 different machines, where running make test will consume all available system memory then crash with an error like:

Starting vector_elasticsearch_1 ... done
   Compiling vector v0.8.0 (/home/leshow/dev/rust/vector)
error: linking with `cc` failed: exit code: 1
  |
  = note: "cc" "-Wl,--as-needed" "-Wl,-z,noexecstack" "-m64" "-L" "/home/leshow/.rustup/toolchains/1.41.0-x86_64-unknown-linux-gnu/lib/rustlib/x86_64-unknown-linux-gnu/lib" "/home/leshow/dev/rust/vector/target/debug/deps/vector-a2c9ee1397164ad8.103giqk6wtmhpcv6.rcgu.o" "/home/leshow/dev/rust/vector/target/debug/deps/vector-a2c9ee1397164ad8.13obbnmdfykyhc3l.rcgu.o" "/home/leshow/dev/rust/vector/target/debug/deps/vector-a2c9ee1397164ad8.14zo4roytzzhj6mi.rcgu.o" "/home/leshow/dev/rust/vector/target/debug/deps/vector-a2c9ee1397164ad8.15lz2hk08g91v568.rcgu.o" "/home/leshow/dev/rust/vector/target/debug/deps/vector-a2c9ee1397164ad8.16whzk623alwv7i8.rcgu.o" "/home/leshow/dev/rust/vector/target/debug/deps/vector-a2c9ee1397164ad8.16y5fyibfga3p1jo.rcgu.o" "/home/leshow/dev/rust/vector/target/debug/deps/vector-a2c9ee1397164ad8.19h3e3n1uyqbaszt.rcgu.o" "/home/leshow/dev/rust/vector/target/debug/deps/vector-a2c9ee1397164ad8.19kbg2dommqxb82f.rcgu.o" "/home/leshow/dev/rust/vector/target/debug/deps/vector-a2c9ee1397164ad8.19kc5nhqnompj6au.rcgu.o" "/home/leshow/dev/rust/vector/target/debug/deps/vector-a2c9ee1397164ad8.1a7fu4u3qti5a3jv.rcgu.o" "/home/leshow/dev/rust/vector/target/debug/deps/vector-a2c9ee1397164ad8.1hq4w0rrd58d3n8s.rcgu.o" "/home/leshow/dev/rust/vector/target/debug/deps/vector-a2c9ee1397164ad8.1kgz424er569b6k.rcgu.o" "/home/leshow/dev/rust/vector/target/debug/deps/vector-a2c9ee1397164ad8.1l3ufz9yktzay4at.rcgu.o" "/home/leshow/dev/rust/vector/target/debug/deps/vector-a2c9ee1397164ad8.1l3x534mfyibiaa8.rcgu.o" "/home/leshow/dev/rust/vector/target/debug/deps/vector-a2c9ee1397164ad8.1ohibu38sjtvp7dj.rcgu.o" "/home/leshow/dev/rust/vector/target/debug/deps/vector-a2c9ee1397164ad8.1prpmiq1z5kawlhi.rcgu.o" "/home/leshow/dev/rust/vector/target/debug/deps/vector-a2c9ee1397164ad8.1qzrfow0172vxc85.rcgu.o" "/home/leshow/dev/rust/vector/target/debug/deps/vector-a2c9ee1397164ad8.1tbyvnu4qugdayz1.rcgu.o" "/home/leshow/dev/rust/vector/target/debug/deps/vector-a2c9ee1397164ad8.1vvmc14v4jr6rfa2.rcgu.o" "/home/leshow/dev/rust/vector/target/debug/deps/vector-a2c9ee1397164ad8.1vxpu4w64y2rr5y5.rcgu.o" "/home/leshow/dev/rust/vector/tar 

...

Has anyone had a problem like this?

bruceg · 2020-03-17T17:45:02Z

@Hoverbear you had requested some changes previously. Are your concerns handled now?

Hoverbear · 2020-03-17T17:46:12Z

I think it's good. :)

leshow · 2020-03-17T17:53:02Z

Sorry I took so much of your time working through the build issues. If you start moving things to async/await and need some help feel free to ping me, I can pick up a few things.

Hoverbear · 2020-03-17T18:36:51Z

Thank you so much @leshow , we are starting to move to that! You can see #1922 for some details about that and @LucioFranco or @MOZGIII may be able to direct your efforts!

Signed-off-by: binarylogic <[email protected]>

binarylogic

Nice work! Thanks.

bruceg · 2020-03-17T19:16:03Z

While looking into the units of the batch size, we discovered that it appears to be disconnected the pulsar crate (it is only implemented for the consumer). Have you tested that the batch size is actually constrained by the option? Did we guess right that it is supposed to be a number of messages?

Obviously if the option is disconnected in the upstream crate, this isn't your problem, but we should not claim to support it then.

leshow · 2020-03-17T21:17:35Z

I just looked again and I think you're correct, the batch size only looks like it's being used for consumers, I'll remove it as an option.

Signed-off-by: Evan Cameron <[email protected]>

bruceg · 2020-03-20T03:14:19Z

Thanks a lot for your work on this @leshow! We're happy to add your contribution.

binarylogic · 2020-03-20T03:25:38Z

Agree! Thank you for pushing this to the finish line.

KannarFr · 2020-03-20T14:25:57Z

Yey! Thanks!

KannarFr · 2020-03-20T14:27:41Z

@bruceg do you plan to release with pulsar sink soon?

binarylogic · 2020-03-20T14:40:08Z

0.10.0 should go out next week pending benchmark results on our test harness

KannarFr · 2020-03-30T03:44:47Z

@binarylogic

Thanks for you answer, any news? Where are benchmark output?

KannarFr · 2020-03-31T07:54:32Z

@leshow

Documentation missing these fields on pulsar sink configuration:

  type = "pulsar" # required
  inputs = ["source_pulsar_sink"] # required

And, I've got the following error:

kannar@pond ~/git/cc/conf/vector % ../../vector/target/debug/vector validate vector.toml                                                                                            
Mar 31 09:46:48.024 ERROR vector: Configuration error: unknown variant `codec`, expected `text` or `json` for key `sinks.pulsar_output`
Mar 31 09:46:48.024 ERROR vector: Failed to parse config file. path="vector.toml"

When using the configuration:

[sinks.pulsar_output]
  type = "pulsar" # required
  inputs = ["input"] # required
  address = "address" # required
  topic = "topic" # required

  # Auth
  auth.name = "${AUTH_PROVIDER}" # optional, no default
  auth.token = "${AUTH_TOKEN}" # optional, no default

  encoding.codec = "text" # required

As I'm running master maybe it's not related to your code.

binarylogic · 2020-03-31T14:03:03Z

Thanks. I've fixed the documentation errors, they will be merged today. And we're planning to release 0.9.0 this week. Benchmarks look good.

Where are benchmark output?

We use to run a variety of tests via https://github.com/timberio/vector-test-harness. All performance data is stored in a public S# bucket as documented in the readme. Hope that helps!

KannarFr · 2020-03-31T21:37:38Z

@binarylogic thanks for your answer.

Another point: address seems to must be an ip, can it be an hostname too?

leshow · 2020-04-01T02:26:11Z

@KannarFr have a look at the docs for pulsar-rs, the address is passed off to them.

KannarFr · 2020-04-03T06:46:07Z

Got it, will contribute to pulsar-rs :).

KannarFr · 2020-04-09T14:00:58Z

@binarylogic any news for 0.9.0?

LucioFranco · 2020-04-09T23:14:09Z

@KannarFr should be coming very very soon :)

KannarFr · 2020-04-15T13:20:09Z

Ahaha, I'm sorry but I need this tag to define the exherbo.org package using the new pulsar sink. Maybe can you release a 0.9.0 with it and a 0.10.0 very very soon?

KannarFr · 2020-04-15T13:42:58Z

@LucioFranco ^

LucioFranco · 2020-04-15T15:37:35Z

I am not totally sure what the status on 0.10 is, though I think @binarylogic currently working on 0.9 as we speak.

leshow requested a review from lukesteensen as a code owner January 31, 2020 20:51

binarylogic changed the title ~~WIP: feat(pulsar sink): add pulsar sink~~ WIP: feat(new sink): Initial pulsar sink implementation Jan 31, 2020

binarylogic requested review from bruceg and removed request for lukesteensen January 31, 2020 21:59

binarylogic assigned bruceg Jan 31, 2020

bruceg reviewed Feb 1, 2020

View reviewed changes

LucioFranco reviewed Feb 4, 2020

View reviewed changes

leshow changed the title ~~WIP: feat(new sink): Initial pulsar sink implementation~~ feat(new sink): pulsar sink implementation Feb 7, 2020

leshow changed the title ~~feat(new sink): pulsar sink implementation~~ feat(new sink): Pulsar sink implementation Feb 7, 2020

binarylogic requested a review from bruceg February 7, 2020 16:47

binarylogic added the needs: docs Needs documentation updates label Feb 9, 2020

binarylogic changed the title ~~feat(new sink): Pulsar sink implementation~~ feat(new sink): Initial pulsar sink implementation Feb 10, 2020

Hoverbear suggested changes Feb 13, 2020

View reviewed changes

Hoverbear approved these changes Mar 17, 2020

View reviewed changes

binarylogic added 2 commits March 17, 2020 14:47

Merge remote-tracking branch 'upstream/master'

b250320

Update docs

b00154f

Signed-off-by: binarylogic <[email protected]>

binarylogic approved these changes Mar 17, 2020

View reviewed changes

bruceg removed the needs: docs Needs documentation updates label Mar 17, 2020

Remove batch

5a3dc66

Signed-off-by: Evan Cameron <[email protected]>

bruceg merged commit c8b47b3 into vectordotdev:master Mar 20, 2020

binarylogic mentioned this pull request Apr 3, 2020

For the pulsar sink, document that the address must be an IP address, not a hostname #2214

Closed

feat(new sink): Initial pulsar sink implementation #1665

feat(new sink): Initial pulsar sink implementation #1665

Conversation

leshow commented Jan 31, 2020

binarylogic commented Jan 31, 2020

binarylogic commented Jan 31, 2020

bruceg left a comment

Choose a reason for hiding this comment

bruceg Feb 1, 2020

Choose a reason for hiding this comment

bruceg Feb 1, 2020

Choose a reason for hiding this comment

bruceg commented Feb 1, 2020

bruceg commented Feb 1, 2020

leshow commented Feb 1, 2020

KannarFr commented Feb 3, 2020

leshow commented Feb 3, 2020

KannarFr commented Feb 3, 2020 via email

KannarFr commented Feb 3, 2020

LucioFranco left a comment

Choose a reason for hiding this comment

leshow commented Feb 6, 2020

binarylogic commented Feb 6, 2020

leshow commented Feb 7, 2020 • edited Loading

leshow commented Feb 8, 2020

binarylogic commented Feb 10, 2020

KannarFr commented Feb 12, 2020 • edited Loading

leshow commented Feb 13, 2020 • edited Loading

Hoverbear Feb 8, 2020

Choose a reason for hiding this comment

leshow Feb 13, 2020

Choose a reason for hiding this comment

leshow commented Feb 14, 2020 • edited Loading

bruceg commented Mar 17, 2020

Hoverbear commented Mar 17, 2020

leshow commented Mar 17, 2020

Hoverbear commented Mar 17, 2020

binarylogic left a comment

Choose a reason for hiding this comment

bruceg commented Mar 17, 2020

leshow commented Mar 17, 2020 • edited Loading

bruceg commented Mar 20, 2020

binarylogic commented Mar 20, 2020

KannarFr commented Mar 20, 2020

KannarFr commented Mar 20, 2020

binarylogic commented Mar 20, 2020

KannarFr commented Mar 30, 2020

KannarFr commented Mar 31, 2020

binarylogic commented Mar 31, 2020

KannarFr commented Mar 31, 2020 • edited Loading

leshow commented Apr 1, 2020

KannarFr commented Apr 3, 2020

KannarFr commented Apr 9, 2020

LucioFranco commented Apr 9, 2020

KannarFr commented Apr 15, 2020

KannarFr commented Apr 15, 2020

LucioFranco commented Apr 15, 2020

feat(new sink): Initial `pulsar` sink implementation #1665

feat(new sink): Initial `pulsar` sink implementation #1665

leshow commented Feb 7, 2020 •

edited

Loading

KannarFr commented Feb 12, 2020 •

edited

Loading

leshow commented Feb 13, 2020 •

edited

Loading

leshow commented Feb 14, 2020 •

edited

Loading

leshow commented Mar 17, 2020 •

edited

Loading

KannarFr commented Mar 31, 2020 •

edited

Loading