-
Notifications
You must be signed in to change notification settings - Fork 29.3k
[SPARK-4964] [Streaming] Exactly-once semantics for Kafka #3798
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from 19 commits
76913e2
1d70625
0b94b33
4dafd1b
ce91c59
7d050bc
783b477
29c6b43
3c2a96a
4b078bf
8d7de4a
979da25
38bb727
326ff3c
6bf14f2
bcca8a4
37d3053
cac63ee
e09045b
8bfd6c0
1d50749
adf99a6
356c7cc
e93eb72
e86317b
0458e4e
548d529
c1bd6d9
d4a7cf7
bb80bbe
2e67117
19406cc
99d2eba
80fd6ae
2b340d8
9a838c2
0090553
9adaa0a
4354bce
825110f
8991017
0df3ebe
8c31855
59e29f6
1dc2941
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -44,7 +44,7 @@ | |
| <dependency> | ||
| <groupId>org.apache.kafka</groupId> | ||
| <artifactId>kafka_${scala.binary.version}</artifactId> | ||
| <version>0.8.0</version> | ||
| <version>0.8.1.1</version> | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why was this necessary? What aspect of this PR depends on this updated version?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's due to the consumer offset management api only being available in 0.8.1.1
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @huitseeker @koeninger @tdas I do have the initial Kafka 0.8.2 PR in, just waiting to update the version to GA vs beta and re-test and check for any changes/regression. |
||
| <exclusions> | ||
| <exclusion> | ||
| <groupId>com.sun.jmx</groupId> | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,305 @@ | ||
| /* | ||
| * Licensed to the Apache Software Foundation (ASF) under one or more | ||
| * contributor license agreements. See the NOTICE file distributed with | ||
| * this work for additional information regarding copyright ownership. | ||
| * The ASF licenses this file to You under the Apache License, Version 2.0 | ||
| * (the "License"); you may not use this file except in compliance with | ||
| * the License. You may obtain a copy of the License at | ||
| * | ||
| * http://www.apache.org/licenses/LICENSE-2.0 | ||
| * | ||
| * Unless required by applicable law or agreed to in writing, software | ||
| * distributed under the License is distributed on an "AS IS" BASIS, | ||
| * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. | ||
| * See the License for the specific language governing permissions and | ||
| * limitations under the License. | ||
| */ | ||
|
|
||
| package org.apache.spark.rdd.kafka | ||
|
|
||
| import scala.util.control.NonFatal | ||
| import scala.collection.mutable.ArrayBuffer | ||
| import java.util.Properties | ||
| import kafka.api._ | ||
| import kafka.common.{ErrorMapping, OffsetMetadataAndError, TopicAndPartition} | ||
| import kafka.consumer.{ConsumerConfig, SimpleConsumer} | ||
|
|
||
| /** | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. can you turn all scaladoc style into javadoc for this pr? |
||
| * Convenience methods for interacting with a Kafka cluster. | ||
| * @param kafkaParams Kafka <a href="http://kafka.apache.org/documentation.html#configuration"> | ||
| * configuration parameters</a>. | ||
| * Requires "metadata.broker.list" or "bootstrap.servers" to be set with Kafka broker(s), | ||
| * NOT zookeeper servers, specified in host1:port1,host2:port2 form | ||
| */ | ||
| class KafkaCluster(val kafkaParams: Map[String, String]) { | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It would probably be good to make this
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The rdd would be really unpleasant to actually use without the convenience methods exposed by KafkaCluster, especially if you're keeping your offsets in zookeeper and doing idempotent writes.
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see - sorry let me look more, I didn't realize this is necessary for users.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. for example We also use it for doing things like e.g. starting a stream at the leader offsets before a given time |
||
| import KafkaCluster.Err | ||
|
|
||
| val seedBrokers: Array[(String, Int)] = | ||
| kafkaParams.get("metadata.broker.list") | ||
| .orElse(kafkaParams.get("bootstrap.servers")) | ||
| .getOrElse(throw new Exception("Must specify metadata.broker.list or bootstrap.servers")) | ||
| .split(",").map { hp => | ||
| val hpa = hp.split(":") | ||
| (hpa(0), hpa(1).toInt) | ||
| } | ||
|
|
||
| val config: ConsumerConfig = KafkaCluster.consumerConfig(kafkaParams) | ||
|
|
||
| def connect(host: String, port: Int): SimpleConsumer = | ||
| new SimpleConsumer(host, port, config.socketTimeoutMs, | ||
| config.socketReceiveBufferBytes, config.clientId) | ||
|
|
||
| def connect(hostAndPort: (String, Int)): SimpleConsumer = | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. better remove this method since it doesn't do much ... |
||
| connect(hostAndPort._1, hostAndPort._2) | ||
|
|
||
| def connectLeader(topic: String, partition: Int): Either[Err, SimpleConsumer] = | ||
| findLeader(topic, partition).right.map(connect) | ||
|
|
||
| def findLeader(topic: String, partition: Int): Either[Err, (String, Int)] = { | ||
| val req = TopicMetadataRequest(TopicMetadataRequest.CurrentVersion, | ||
| 0, config.clientId, Seq(topic)) | ||
| val errs = new Err | ||
| withBrokers(seedBrokers, errs) { consumer => | ||
| val resp: TopicMetadataResponse = consumer.send(req) | ||
| resp.topicsMetadata.find(_.topic == topic).flatMap { t => | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this block of code is pretty hard to understand with so many level of nesting. can you rewrite it? maybe by introducing variables and adding comments to explain what is going on. overall I feel this PR went slightly overboard with Scala. With no explicit type, intermediate variable, and comment, it is pretty hard to understand a lot of blocks |
||
| t.partitionsMetadata.find(_.partitionId == partition) | ||
| }.foreach { partitionMeta => | ||
| partitionMeta.leader.foreach { leader => | ||
| return Right((leader.host, leader.port)) | ||
| } | ||
| } | ||
| } | ||
| Left(errs) | ||
| } | ||
|
|
||
| def findLeaders( | ||
| topicAndPartitions: Set[TopicAndPartition] | ||
| ): Either[Err, Map[TopicAndPartition, (String, Int)]] = { | ||
| getPartitionMetadata(topicAndPartitions.map(_.topic)).right.flatMap { tms => | ||
| val result = tms.flatMap { tm: TopicMetadata => | ||
| tm.partitionsMetadata.flatMap { pm => | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this block also |
||
| val tp = TopicAndPartition(tm.topic, pm.partitionId) | ||
| if (topicAndPartitions(tp)) { | ||
| pm.leader.map { l => | ||
| tp -> (l.host -> l.port) | ||
| } | ||
| } else { | ||
| None | ||
| } | ||
| } | ||
| }.toMap | ||
| if (result.keys.size == topicAndPartitions.size) { | ||
| Right(result) | ||
| } else { | ||
| val missing = topicAndPartitions.diff(result.keys.toSet) | ||
| val err = new Err | ||
| err.append(new Exception(s"Couldn't find leaders for ${missing}")) | ||
| Left(err) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| def getPartitions(topics: Set[String]): Either[Err, Set[TopicAndPartition]] = | ||
| getPartitionMetadata(topics).right.map { r => | ||
| r.flatMap { tm: TopicMetadata => | ||
| tm.partitionsMetadata.map { pm => | ||
| TopicAndPartition(tm.topic, pm.partitionId) | ||
| } | ||
| } | ||
| } | ||
|
|
||
| def getPartitionMetadata(topics: Set[String]): Either[Err, Set[TopicMetadata]] = { | ||
| val req = TopicMetadataRequest(TopicMetadataRequest.CurrentVersion, | ||
| 0, config.clientId, topics.toSeq) | ||
| val errs = new Err | ||
| withBrokers(seedBrokers, errs) { consumer => | ||
| val resp: TopicMetadataResponse = consumer.send(req) | ||
| // error codes here indicate missing / just created topic, | ||
| // repeating on a different broker wont be useful | ||
| return Right(resp.topicsMetadata.toSet) | ||
| } | ||
| Left(errs) | ||
| } | ||
|
|
||
| def getLatestLeaderOffsets( | ||
| topicAndPartitions: Set[TopicAndPartition] | ||
| ): Either[Err, Map[TopicAndPartition, Long]] = | ||
| getLeaderOffsets(topicAndPartitions, OffsetRequest.LatestTime) | ||
|
|
||
| def getEarliestLeaderOffsets( | ||
| topicAndPartitions: Set[TopicAndPartition] | ||
| ): Either[Err, Map[TopicAndPartition, Long]] = | ||
| getLeaderOffsets(topicAndPartitions, OffsetRequest.EarliestTime) | ||
|
|
||
| def getLeaderOffsets( | ||
| topicAndPartitions: Set[TopicAndPartition], | ||
| before: Long | ||
| ): Either[Err, Map[TopicAndPartition, Long]] = | ||
| getLeaderOffsets(topicAndPartitions, before, 1).right.map { r => | ||
| r.map { kv => | ||
| // mapValues isnt serializable, see SI-7005 | ||
| kv._1 -> kv._2.head | ||
| } | ||
| } | ||
|
|
||
| private def flip[K, V](m: Map[K, V]): Map[V, Seq[K]] = | ||
| m.groupBy(_._2).map { kv => | ||
| kv._1 -> kv._2.keys.toSeq | ||
| } | ||
|
|
||
| def getLeaderOffsets( | ||
| topicAndPartitions: Set[TopicAndPartition], | ||
| before: Long, | ||
| maxNumOffsets: Int | ||
| ): Either[Err, Map[TopicAndPartition, Seq[Long]]] = { | ||
| findLeaders(topicAndPartitions).right.flatMap { tpToLeader => | ||
| val leaderToTp: Map[(String, Int), Seq[TopicAndPartition]] = flip(tpToLeader) | ||
| val leaders = leaderToTp.keys | ||
| var result = Map[TopicAndPartition, Seq[Long]]() | ||
| val errs = new Err | ||
| withBrokers(leaders, errs) { consumer => | ||
| val needed: Seq[TopicAndPartition] = leaderToTp((consumer.host, consumer.port)) | ||
| val req = OffsetRequest( | ||
| needed.map { tp => | ||
| tp -> PartitionOffsetRequestInfo(before, maxNumOffsets) | ||
| }.toMap | ||
| ) | ||
| val resp = consumer.getOffsetsBefore(req) | ||
| val respMap = resp.partitionErrorAndOffsets | ||
| needed.foreach { tp => | ||
| respMap.get(tp).foreach { errAndOffsets => | ||
| if (errAndOffsets.error == ErrorMapping.NoError) { | ||
| if (errAndOffsets.offsets.nonEmpty) { | ||
| result += tp -> errAndOffsets.offsets | ||
| } else { | ||
| errs.append(new Exception( | ||
| s"Empty offsets for ${tp}, is ${before} before log beginning?")) | ||
| } | ||
| } else { | ||
| errs.append(ErrorMapping.exceptionFor(errAndOffsets.error)) | ||
| } | ||
| } | ||
| } | ||
| if (result.keys.size == topicAndPartitions.size) { | ||
| return Right(result) | ||
| } | ||
| } | ||
| val missing = topicAndPartitions.diff(result.keys.toSet) | ||
| errs.append(new Exception(s"Couldn't find leader offsets for ${missing}")) | ||
| Left(errs) | ||
| } | ||
| } | ||
|
|
||
| def getConsumerOffsets( | ||
| groupId: String, | ||
| topicAndPartitions: Set[TopicAndPartition] | ||
| ): Either[Err, Map[TopicAndPartition, Long]] = { | ||
| getConsumerOffsetMetadata(groupId, topicAndPartitions).right.map { r => | ||
| r.map { kv => | ||
| kv._1 -> kv._2.offset | ||
| } | ||
| } | ||
| } | ||
|
|
||
| def getConsumerOffsetMetadata( | ||
| groupId: String, | ||
| topicAndPartitions: Set[TopicAndPartition] | ||
| ): Either[Err, Map[TopicAndPartition, OffsetMetadataAndError]] = { | ||
| var result = Map[TopicAndPartition, OffsetMetadataAndError]() | ||
| val req = OffsetFetchRequest(groupId, topicAndPartitions.toSeq) | ||
| val errs = new Err | ||
| withBrokers(seedBrokers, errs) { consumer => | ||
| val resp = consumer.fetchOffsets(req) | ||
| val respMap = resp.requestInfo | ||
| val needed = topicAndPartitions.diff(result.keys.toSet) | ||
| needed.foreach { tp => | ||
| respMap.get(tp).foreach { offsetMeta => | ||
| if (offsetMeta.error == ErrorMapping.NoError) { | ||
| result += tp -> offsetMeta | ||
| } else { | ||
| errs.append(ErrorMapping.exceptionFor(offsetMeta.error)) | ||
| } | ||
| } | ||
| } | ||
| if (result.keys.size == topicAndPartitions.size) { | ||
| return Right(result) | ||
| } | ||
| } | ||
| val missing = topicAndPartitions.diff(result.keys.toSet) | ||
| errs.append(new Exception(s"Couldn't find consumer offsets for ${missing}")) | ||
| Left(errs) | ||
| } | ||
|
|
||
| def setConsumerOffsets( | ||
| groupId: String, | ||
| offsets: Map[TopicAndPartition, Long] | ||
| ): Either[Err, Map[TopicAndPartition, Short]] = { | ||
| setConsumerOffsetMetadata(groupId, offsets.map { kv => | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do a { case (k,v) => vs accessing the tuples as ._1 etc |
||
| kv._1 -> OffsetMetadataAndError(kv._2) | ||
| }) | ||
| } | ||
|
|
||
| def setConsumerOffsetMetadata( | ||
| groupId: String, | ||
| metadata: Map[TopicAndPartition, OffsetMetadataAndError] | ||
| ): Either[Err, Map[TopicAndPartition, Short]] = { | ||
| var result = Map[TopicAndPartition, Short]() | ||
| val req = OffsetCommitRequest(groupId, metadata) | ||
| val errs = new Err | ||
| val topicAndPartitions = metadata.keys.toSet | ||
| withBrokers(seedBrokers, errs) { consumer => | ||
| val resp = consumer.commitOffsets(req) | ||
| val respMap = resp.requestInfo | ||
| val needed = topicAndPartitions.diff(result.keys.toSet) | ||
| needed.foreach { tp => | ||
| respMap.get(tp).foreach { err => | ||
| if (err == ErrorMapping.NoError) { | ||
| result += tp -> err | ||
| } else { | ||
| errs.append(ErrorMapping.exceptionFor(err)) | ||
| } | ||
| } | ||
| } | ||
| if (result.keys.size == topicAndPartitions.size) { | ||
| return Right(result) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. return? how about just
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That's an early return from withBrokers, which would otherwise keep running the closure on each broker. |
||
| } | ||
| } | ||
| val missing = topicAndPartitions.diff(result.keys.toSet) | ||
| errs.append(new Exception(s"Couldn't set offsets for ${missing}")) | ||
| Left(errs) | ||
| } | ||
|
|
||
| private def withBrokers(brokers: Iterable[(String, Int)], errs: Err) | ||
| (fn: SimpleConsumer => Any): Unit = { | ||
| brokers.foreach { hp => | ||
| var consumer: SimpleConsumer = null | ||
| try { | ||
| consumer = connect(hp) | ||
| fn(consumer) | ||
| } catch { | ||
| case NonFatal(e) => | ||
| errs.append(e) | ||
| } finally { | ||
| if (consumer != null) consumer.close() | ||
| } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Or you can stay away from java nulls
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I appreciate the feedback, but it's impossible to "stay away from java nulls" in a jvm language, without runtime checks. Despite propaganda to the contrary, option.map is not a replacement for null checks. The code you wrote can still throw a null pointer exception (if SimpleConsumer returns null, for instance). You can hide the null pointer check inside of Option.apply instead of using Some, but at that point I'd rather just be explicit about what is going on. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. True, it could because I was not conclusive on the suggestion adding None where applicable. catch => None
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. the use of null here is fine, and very clear. the pattern matching with the finally actually makes it much harder to understand what is going on. one nitpick, you need to put curly braces around consumer.close(), i.e. if (consumer != null) {
consumer.close()
} |
||
| } | ||
| } | ||
| } | ||
|
|
||
| object KafkaCluster { | ||
| type Err = ArrayBuffer[Throwable] | ||
|
|
||
| /** Make a consumer config without requiring group.id or zookeeper.connect, | ||
| * since communicating with brokers also needs common settings such as timeout | ||
| */ | ||
| def consumerConfig(kafkaParams: Map[String, String]): ConsumerConfig = { | ||
| val props = new Properties() | ||
| kafkaParams.foreach(param => props.put(param._1, param._2)) | ||
| Seq("zookeeper.connect", "group.id").foreach { s => | ||
| if (!props.contains(s)) { | ||
| props.setProperty(s, "") | ||
| } | ||
| } | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. containsKey is less expensive than contains
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I wouldn't use the collect (even though it allows you to do the partial function) because it is actually obfuscating the intention here, because it is perceived as a filter. foreach makes more sense. |
||
| new ConsumerConfig(props) | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This isnt being used anywhere, is it ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The code for actually using the rdd and committing offsets transactionally is quite awkward without that method, see
koeninger/kafka-exactly-once@cb812c9
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, this is not necessary for this particular PR, isnt it? Then no point introducing it in this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Are you saying that you would prefer for users of this class to have to use mapPartitionsWithIndex with a side-effect of storing data, and then add an empty foreach in order to trigger the job to actually be scheduled?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No no, I am not suggesting anything. I am just narrowing the scope the PR by eliminating anything that is not immediately needed in this PR (basic Kafka functionality). There is already a lot more code necessary for this to be viable completely alternative (Driver fault-tolerance, etc.) and it would be good to focus on only that in this PR. The
foreachPartitionWithIndexseems like something that can be done later independently in a different PR.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I disagree -- I think it should stay here. It seems like a pretty obvious omission from the RDD api. Probably its never been added b/c there wasn't a good use case. Well, now we've got a good use case. IMO no sense in creating another PR that needs review just for that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can't this be done with just mapPartitionsWithContext followed by a foreach? I feel we are adding all kinds of random APIs to RDD that is making it very hard to maintain. If it is completely up to me, I'd even remove a bunch of existing RDD APIs ...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, I was originally doing map followed by empty foreach and thought it looked confusing.
I think it's really a question of what's easier to explain, it's just a syntax sugar issue not a correctness issue, so no problem either way.