search

Netflix / suro

Netflix's distributed Data Pipeline
Apache License 2.0
794 stars 170 forks source link

added KafkaV2 sink using new kafka producer in 0.8.2-SNAPSHOT #127

Closed starzia closed 9 years ago

starzia commented 10 years ago

@metacret here is the pull request that we discussed. Please see suro-server/conf/sink_using_kafkaV2.json for the configuration syntax.

cloudbees-pull-request-builder commented 10 years ago

suro-pull-requests #95 FAILURE Looks like there's a problem with this pull request

metacret commented 10 years ago

licenseTest was timed out. This PR doesn't have any unit testing and it should be fine.

starzia commented 10 years ago

@metacret , we also noticed that the stats published at http://localhost:7103/surosinkstat are broken. I'll send another pull request when these two issues are fixed.

metacret commented 10 years ago

Even though KafkaSinkV2 implementation is trivial, could you write the unit test?

cloudbees-pull-request-builder commented 10 years ago

suro-pull-requests #96 FAILURE Looks like there's a problem with this pull request

cloudbees-pull-request-builder commented 10 years ago

suro-pull-requests #97 FAILURE Looks like there's a problem with this pull request

metacret commented 10 years ago

@starzia Thanks a lot for your new commits. We're almost done! I left a few comments and could you fix them? Also, PR is failing due to licenseTest timeout, I asked Netflix tools team and it will be fixed soon. Do not care about PR failure too much for a while.

metacret commented 10 years ago

@starzia Netflix tools team told me the timeout seems happening on the test. Could you check on your local repository whether all tests are fine?

starzia commented 10 years ago

I'll be able to check on Monday. Thanks.

On Sep 19, 2014, at 12:55 PM, Jae Hyeon Bae notifications@github.com wrote:

@starzia Netflix tools team told me the timeout seems happening on the test. Could you check on your local repository whether all tests are fine?

— Reply to this email directly or view it on GitHub.

metacret commented 10 years ago

@starzia May I ask you one more fix?

Even though I didn't see much difference between sync mode and async mode of new producer, we'd better to have both of them as optional. In your PR, you're calling get() method for every Future returned by send() method. This will block the operation in the Runnable. So that's why we would need to remove ThreadPool and let KafkaProducer do the non-blocking IO. What do you think?

starzia commented 9 years ago

@metacret, the tests are working fine here. My latest commit fixes the partitioning backward compatibility and key storage issue. I forgot to look at the async mode. I will look at that now.

cloudbees-pull-request-builder commented 9 years ago

suro-pull-requests #100 FAILURE Looks like there's a problem with this pull request

starzia commented 9 years ago

@metacret I think it's better to force the Kafka producer to use synchronous mode because then the user does not have to worry about another queue (the internal Kafka queue). This would be a fourth queue (we already have the thrift input buffer, the Message Set Processor queue, and the sink queue). Do you have a use case in mind for the async mode?

One thing we may want to add is an option to disable requeueing of failed messages in KafkaSinkV2. This can cause an infinite buildup of messages if Kafka is down (which is exactly what I want in my use case). Let me know what you think.

Also, about the tests, they took 13 minutes on my machine so maybe that is why your tools team is seeing a timeout?

cloudbees-pull-request-builder commented 9 years ago

suro-pull-requests #101 FAILURE Looks like there's a problem with this pull request

metacret commented 9 years ago

OK, regarding asynchronous one, I agree with you. I will take a look at why this PR keeps failing and merge it.

cloudbees-pull-request-builder commented 9 years ago

suro-pull-requests #102 FAILURE Looks like there's a problem with this pull request

cloudbees-pull-request-builder commented 9 years ago

suro-pull-requests #103 FAILURE Looks like there's a problem with this pull request

metacret commented 9 years ago

@starzia Could you merge trunk and update this PR? I mistakenly merged one PR which contains the test which keeps failing.

starzia commented 9 years ago

@metacret done.

cloudbees-pull-request-builder commented 9 years ago

suro-pull-requests #105 SUCCESS This pull request looks good