search

sclasen / akka-kafka

185 stars 62 forks source link

AkkaHighLevelConsumer additions #44

Closed coreyauger closed 8 years ago

coreyauger commented 8 years ago

Can you add an example of what you are thinking for I think it would be clearer if you just submit runnables to the Executor and I will add it and do another push. Thanks :)

sclasen commented 8 years ago

@coreyauger not sure if you saw I did a PR on your branch to show what I menat

coreyauger commented 8 years ago

Hey .. for the late reply. I have been in transit most of the weekend. I will merge those changes and flatten the commit into a single commit for a nicer history :)

coreyauger commented 8 years ago

Spoke to soon. I don't see the PR on any of my branches for this project? Am I missing something?

sclasen commented 8 years ago

Grr some sort of github ui fail. Can't find it on github either

Will re submit it or gist something up and get back to you, sorry for the noise

On Sunday, February 14, 2016, Corey Auger notifications@github.com wrote:

Spoke to soon. I don't see the PR on any of my branches for this project? Am I missing something?

— Reply to this email directly or view it on GitHub https://github.com/sclasen/akka-kafka/pull/44#issuecomment-184058920.

coreyauger commented 8 years ago

Hehe.. no problem. I have a love hate relationship with git myself :)

sclasen commented 8 years ago

https://github.com/sclasen/akka-kafka/commit/2fc20e24ba5d178792d697d518a0a7d463a868dd

coreyauger commented 8 years ago

How are we looking ?

sclasen commented 8 years ago

Looking good, can you add a basic test ?

They are in 'src/it'

coreyauger commented 8 years ago

sorry been super busy.. I will try add the test tomorrow :)

coreyauger commented 8 years ago

Sorry this is going to be a dumb question but how do i run the test from 'src/it'
I usually have them in 'scala/test' and can just run 'test' from sbt.

Thanks.

sclasen commented 8 years ago

hehe sorry bout the lack of doc there sbt it:test will run the tests. you need kafka and zookeeper running locally

On Fri, Feb 19, 2016 at 1:34 PM, Corey Auger notifications@github.com wrote:

Sorry this is going to be a dumb question but how do i run the test from 'src/it'

I usually have them in 'scala/test' and can just run 'test' from sbt.

Thanks.

— Reply to this email directly or view it on GitHub https://github.com/sclasen/akka-kafka/pull/44#issuecomment-186417162.

coreyauger commented 8 years ago

There are some issues when I run the test. It seems that my test will pass while others will fail.. is this because of a config issue ?

sclasen commented 8 years ago

if your test passes, then thats sufficient. the tests are a bit flakey, running each by themselves works, but sometimes running all of them fail non-deterministically.

On Mon, Feb 22, 2016 at 9:20 AM, Corey Auger notifications@github.com wrote:

There are some issues when I run the test. It seems that my test will pass while others will fail.. is this because of a config issue ?

— Reply to this email directly or view it on GitHub https://github.com/sclasen/akka-kafka/pull/44#issuecomment-187276434.

coreyauger commented 8 years ago

Ok that is what I was seeing. Would you like me to collapse the commits into a single commit again?

sclasen commented 8 years ago

Nah, 2 commits are fine. Ready to go in your opinion?

On Mon, Feb 22, 2016 at 9:26 AM, Corey Auger notifications@github.com wrote:

Ok that is what I was seeing. Would you like me to collapse the commits into a single commit again?

— Reply to this email directly or view it on GitHub https://github.com/sclasen/akka-kafka/pull/44#issuecomment-187279200.

coreyauger commented 8 years ago

Yes :)

sclasen commented 8 years ago

Thanks, I'll push a release to maven central in a bit here

coreyauger commented 8 years ago

Thanks Scott :)

sclasen commented 8 years ago

@coreyauger just pushed 0.2.0 to maven central, it will show up in a bit.

One thing we missed is fixing the stop method. Right now it is actually blocking, despite it returning a future. It blocks till everything is shut down and then returns a completed future, which isnt what we want.

The whole thing should be in a Future{ } that ends up returning () on its last line?

Make sense?

coreyauger commented 8 years ago

Ah sorry.. are you going to add that then? Or did you want me to do another pr?

sclasen commented 8 years ago

Mind doing a quick PR? I'll do a 2.0.1 when its all set.

On Mon, Feb 22, 2016 at 9:58 AM, Corey Auger notifications@github.com wrote:

Ah sorry.. are you going to add that then? Or did you want me to do another pr?

— Reply to this email directly or view it on GitHub https://github.com/sclasen/akka-kafka/pull/44#issuecomment-187294831.

coreyauger commented 8 years ago

Yep.. :)