Re: Review Request 29912: Patch for KAFKA-1852
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29912/#review74569 --- Ship it! Minor locking issue noted below. I can take care of that. This obviously does not cover the case of committing offsets to a topic that is currently being deleted. I think that can be done in a separate jira. Can you file one? core/src/main/scala/kafka/server/KafkaServer.scala https://reviews.apache.org/r/29912/#comment121175 This can just be a val core/src/main/scala/kafka/server/MetadataCache.scala https://reviews.apache.org/r/29912/#comment121176 Should probably do this inReadLock - Joel Koshy On Feb. 18, 2015, 9:13 p.m., Sriharsha Chintalapani wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29912/ --- (Updated Feb. 18, 2015, 9:13 p.m.) Review request for kafka. Bugs: KAFKA-1852 https://issues.apache.org/jira/browse/KAFKA-1852 Repository: kafka Description --- KAFKA-1852. OffsetCommitRequest can commit offset on unknown topic. Added contains method to MetadataCache. KAFKA-1852. OffsetCommitRequest can commit offset on unknown topic. KAFKA-1852. OffsetCommitRequest can commit offset on unknown topic. Diffs - core/src/main/scala/kafka/server/KafkaApis.scala 703886a1d48e6d2271da67f8b89514a6950278dd core/src/main/scala/kafka/server/KafkaServer.scala 7e5ddcb9be8fcef3df6ebc82a13ef44ef95f73ae core/src/main/scala/kafka/server/MetadataCache.scala 4c70aa7e0157b85de5e24736ebf487239c4571d0 core/src/main/scala/kafka/server/OffsetManager.scala 83d52643028c5628057dc0aa29819becfda61fdb core/src/test/scala/unit/kafka/server/OffsetCommitTest.scala 5b93239cdc26b5be7696f4e7863adb9fbe5f0ed5 Diff: https://reviews.apache.org/r/29912/diff/ Testing --- Thanks, Sriharsha Chintalapani
Re: Review Request 29912: Patch for KAFKA-1852
On Feb. 27, 2015, 8:47 p.m., Joel Koshy wrote: Minor locking issue noted below. I can take care of that. This obviously does not cover the case of committing offsets to a topic that is currently being deleted. I think that can be done in a separate jira. Can you file one? Joel, Will open a new and send PR. Thanks. - Sriharsha --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29912/#review74569 --- On Feb. 27, 2015, 9:50 p.m., Sriharsha Chintalapani wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29912/ --- (Updated Feb. 27, 2015, 9:50 p.m.) Review request for kafka. Bugs: KAFKA-1852 https://issues.apache.org/jira/browse/KAFKA-1852 Repository: kafka Description --- KAFKA-1852. OffsetCommitRequest can commit offset on unknown topic. Added contains method to MetadataCache. KAFKA-1852. OffsetCommitRequest can commit offset on unknown topic. KAFKA-1852. OffsetCommitRequest can commit offset on unknown topic. KAFKA-1852. OffsetCommitRequest can commit offset on unknown topic. Diffs - core/src/main/scala/kafka/server/KafkaApis.scala 703886a1d48e6d2271da67f8b89514a6950278dd core/src/main/scala/kafka/server/KafkaServer.scala 426e522fc9819a0fc0f4e8269033552d716eb066 core/src/main/scala/kafka/server/MetadataCache.scala 4c70aa7e0157b85de5e24736ebf487239c4571d0 core/src/main/scala/kafka/server/OffsetManager.scala 83d52643028c5628057dc0aa29819becfda61fdb core/src/test/scala/unit/kafka/server/OffsetCommitTest.scala a2bb8855c3c0586b6b45b53ce534edee31b3bd12 Diff: https://reviews.apache.org/r/29912/diff/ Testing --- Thanks, Sriharsha Chintalapani
Re: Review Request 29912: Patch for KAFKA-1852
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29912/ --- (Updated Feb. 27, 2015, 9:50 p.m.) Review request for kafka. Bugs: KAFKA-1852 https://issues.apache.org/jira/browse/KAFKA-1852 Repository: kafka Description (updated) --- KAFKA-1852. OffsetCommitRequest can commit offset on unknown topic. Added contains method to MetadataCache. KAFKA-1852. OffsetCommitRequest can commit offset on unknown topic. KAFKA-1852. OffsetCommitRequest can commit offset on unknown topic. KAFKA-1852. OffsetCommitRequest can commit offset on unknown topic. Diffs (updated) - core/src/main/scala/kafka/server/KafkaApis.scala 703886a1d48e6d2271da67f8b89514a6950278dd core/src/main/scala/kafka/server/KafkaServer.scala 426e522fc9819a0fc0f4e8269033552d716eb066 core/src/main/scala/kafka/server/MetadataCache.scala 4c70aa7e0157b85de5e24736ebf487239c4571d0 core/src/main/scala/kafka/server/OffsetManager.scala 83d52643028c5628057dc0aa29819becfda61fdb core/src/test/scala/unit/kafka/server/OffsetCommitTest.scala a2bb8855c3c0586b6b45b53ce534edee31b3bd12 Diff: https://reviews.apache.org/r/29912/diff/ Testing --- Thanks, Sriharsha Chintalapani
Re: Review Request 29912: Patch for KAFKA-1852
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29912/#review74614 --- Ship it! Ship It! - Joel Koshy On Feb. 27, 2015, 9:50 p.m., Sriharsha Chintalapani wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29912/ --- (Updated Feb. 27, 2015, 9:50 p.m.) Review request for kafka. Bugs: KAFKA-1852 https://issues.apache.org/jira/browse/KAFKA-1852 Repository: kafka Description --- KAFKA-1852. OffsetCommitRequest can commit offset on unknown topic. Added contains method to MetadataCache. KAFKA-1852. OffsetCommitRequest can commit offset on unknown topic. KAFKA-1852. OffsetCommitRequest can commit offset on unknown topic. KAFKA-1852. OffsetCommitRequest can commit offset on unknown topic. Diffs - core/src/main/scala/kafka/server/KafkaApis.scala 703886a1d48e6d2271da67f8b89514a6950278dd core/src/main/scala/kafka/server/KafkaServer.scala 426e522fc9819a0fc0f4e8269033552d716eb066 core/src/main/scala/kafka/server/MetadataCache.scala 4c70aa7e0157b85de5e24736ebf487239c4571d0 core/src/main/scala/kafka/server/OffsetManager.scala 83d52643028c5628057dc0aa29819becfda61fdb core/src/test/scala/unit/kafka/server/OffsetCommitTest.scala a2bb8855c3c0586b6b45b53ce534edee31b3bd12 Diff: https://reviews.apache.org/r/29912/diff/ Testing --- Thanks, Sriharsha Chintalapani
Re: Review Request 29912: Patch for KAFKA-1852
On Feb. 13, 2015, 7:01 p.m., Joel Koshy wrote: core/src/main/scala/kafka/server/OffsetManager.scala, line 215 https://reviews.apache.org/r/29912/diff/3/?file=862699#file862699line215 Minor comment. I think this may be better to pass in to the OffsetManager. We should even use it in loadOffsets to discard offsets that are from topics that have been deleted. We can do that in a separate jira - I don't think our handling for clearing out offsets on a delete topic is done yet - Onur Karaman did it for ZK based offsets but we need a separate jira to delete Kafka-based offsets. Sriharsha Chintalapani wrote: Thanks for the review. Since offsetmanager initialized in KafkaServer and metadataCache in KafkaApis , in the latest patch I added setMetadataCache in OffsetManager and calling it in KafkaApis. Please take a look Joel Koshy wrote: In that case I think it is just better to create the cache outside (in KafkaServer and pass it in to KafkaApis). The metadataCache is useful enough to be used in other places (other than just KafkaApis). Updated the patch as per your suggestion. Please take a look. thanks. - Sriharsha --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29912/#review72413 --- On Feb. 18, 2015, 9:13 p.m., Sriharsha Chintalapani wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29912/ --- (Updated Feb. 18, 2015, 9:13 p.m.) Review request for kafka. Bugs: KAFKA-1852 https://issues.apache.org/jira/browse/KAFKA-1852 Repository: kafka Description --- KAFKA-1852. OffsetCommitRequest can commit offset on unknown topic. Added contains method to MetadataCache. KAFKA-1852. OffsetCommitRequest can commit offset on unknown topic. KAFKA-1852. OffsetCommitRequest can commit offset on unknown topic. Diffs - core/src/main/scala/kafka/server/KafkaApis.scala 703886a1d48e6d2271da67f8b89514a6950278dd core/src/main/scala/kafka/server/KafkaServer.scala 7e5ddcb9be8fcef3df6ebc82a13ef44ef95f73ae core/src/main/scala/kafka/server/MetadataCache.scala 4c70aa7e0157b85de5e24736ebf487239c4571d0 core/src/main/scala/kafka/server/OffsetManager.scala 83d52643028c5628057dc0aa29819becfda61fdb core/src/test/scala/unit/kafka/server/OffsetCommitTest.scala 5b93239cdc26b5be7696f4e7863adb9fbe5f0ed5 Diff: https://reviews.apache.org/r/29912/diff/ Testing --- Thanks, Sriharsha Chintalapani
Re: Review Request 29912: Patch for KAFKA-1852
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29912/ --- (Updated Feb. 18, 2015, 9:13 p.m.) Review request for kafka. Bugs: KAFKA-1852 https://issues.apache.org/jira/browse/KAFKA-1852 Repository: kafka Description (updated) --- KAFKA-1852. OffsetCommitRequest can commit offset on unknown topic. Added contains method to MetadataCache. KAFKA-1852. OffsetCommitRequest can commit offset on unknown topic. KAFKA-1852. OffsetCommitRequest can commit offset on unknown topic. Diffs (updated) - core/src/main/scala/kafka/server/KafkaApis.scala 703886a1d48e6d2271da67f8b89514a6950278dd core/src/main/scala/kafka/server/KafkaServer.scala 7e5ddcb9be8fcef3df6ebc82a13ef44ef95f73ae core/src/main/scala/kafka/server/MetadataCache.scala 4c70aa7e0157b85de5e24736ebf487239c4571d0 core/src/main/scala/kafka/server/OffsetManager.scala 83d52643028c5628057dc0aa29819becfda61fdb core/src/test/scala/unit/kafka/server/OffsetCommitTest.scala 5b93239cdc26b5be7696f4e7863adb9fbe5f0ed5 Diff: https://reviews.apache.org/r/29912/diff/ Testing --- Thanks, Sriharsha Chintalapani
Re: Review Request 29912: Patch for KAFKA-1852
On Feb. 13, 2015, 7:01 p.m., Joel Koshy wrote: core/src/main/scala/kafka/server/OffsetManager.scala, line 215 https://reviews.apache.org/r/29912/diff/3/?file=862699#file862699line215 Minor comment. I think this may be better to pass in to the OffsetManager. We should even use it in loadOffsets to discard offsets that are from topics that have been deleted. We can do that in a separate jira - I don't think our handling for clearing out offsets on a delete topic is done yet - Onur Karaman did it for ZK based offsets but we need a separate jira to delete Kafka-based offsets. Sriharsha Chintalapani wrote: Thanks for the review. Since offsetmanager initialized in KafkaServer and metadataCache in KafkaApis , in the latest patch I added setMetadataCache in OffsetManager and calling it in KafkaApis. Please take a look In that case I think it is just better to create the cache outside (in KafkaServer and pass it in to KafkaApis). The metadataCache is useful enough to be used in other places (other than just KafkaApis). - Joel --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29912/#review72413 --- On Feb. 16, 2015, 9:22 p.m., Sriharsha Chintalapani wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29912/ --- (Updated Feb. 16, 2015, 9:22 p.m.) Review request for kafka. Bugs: KAFKA-1852 https://issues.apache.org/jira/browse/KAFKA-1852 Repository: kafka Description --- KAFKA-1852. OffsetCommitRequest can commit offset on unknown topic. Added contains method to MetadataCache. KAFKA-1852. OffsetCommitRequest can commit offset on unknown topic. Diffs - core/src/main/scala/kafka/server/KafkaApis.scala 703886a1d48e6d2271da67f8b89514a6950278dd core/src/main/scala/kafka/server/MetadataCache.scala 4c70aa7e0157b85de5e24736ebf487239c4571d0 core/src/main/scala/kafka/server/OffsetManager.scala 83d52643028c5628057dc0aa29819becfda61fdb core/src/test/scala/unit/kafka/server/OffsetCommitTest.scala 5b93239cdc26b5be7696f4e7863adb9fbe5f0ed5 Diff: https://reviews.apache.org/r/29912/diff/ Testing --- Thanks, Sriharsha Chintalapani
Re: Review Request 29912: Patch for KAFKA-1852
On Feb. 13, 2015, 7:01 p.m., Joel Koshy wrote: core/src/main/scala/kafka/server/OffsetManager.scala, line 215 https://reviews.apache.org/r/29912/diff/3/?file=862699#file862699line215 Minor comment. I think this may be better to pass in to the OffsetManager. We should even use it in loadOffsets to discard offsets that are from topics that have been deleted. We can do that in a separate jira - I don't think our handling for clearing out offsets on a delete topic is done yet - Onur Karaman did it for ZK based offsets but we need a separate jira to delete Kafka-based offsets. Thanks for the review. Since offsetmanager initialized in KafkaServer and metadataCache in KafkaApis , in the latest patch I added setMetadataCache in OffsetManager and calling it in KafkaApis. Please take a look - Sriharsha --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29912/#review72413 --- On Feb. 16, 2015, 9:22 p.m., Sriharsha Chintalapani wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29912/ --- (Updated Feb. 16, 2015, 9:22 p.m.) Review request for kafka. Bugs: KAFKA-1852 https://issues.apache.org/jira/browse/KAFKA-1852 Repository: kafka Description --- KAFKA-1852. OffsetCommitRequest can commit offset on unknown topic. Added contains method to MetadataCache. KAFKA-1852. OffsetCommitRequest can commit offset on unknown topic. Diffs - core/src/main/scala/kafka/server/KafkaApis.scala 703886a1d48e6d2271da67f8b89514a6950278dd core/src/main/scala/kafka/server/MetadataCache.scala 4c70aa7e0157b85de5e24736ebf487239c4571d0 core/src/main/scala/kafka/server/OffsetManager.scala 83d52643028c5628057dc0aa29819becfda61fdb core/src/test/scala/unit/kafka/server/OffsetCommitTest.scala 5b93239cdc26b5be7696f4e7863adb9fbe5f0ed5 Diff: https://reviews.apache.org/r/29912/diff/ Testing --- Thanks, Sriharsha Chintalapani
Re: Review Request 29912: Patch for KAFKA-1852
On Feb. 13, 2015, 6:24 p.m., Eric Olander wrote:
core/src/main/scala/kafka/server/KafkaApis.scala, line 303
https://reviews.apache.org/r/29912/diff/3/?file=862697#file862697line303
This code could be done using map and getOrElse on the Option rather
than using pattern matching/reflection.
logManager.getLog(topicAndPartition).map{log = fetchOffsetsBefore(log,
timestamp, maxNumOffsets)
}.getOrElse {
if (timestamp == OffsetRequest.LatestTime || timestamp ==
OffsetRequest.EarliestTime)
Seq(0L)
else
Nil
}
Sriharsha Chintalapani wrote:
Eric,
Thanks for the review. But the above code is not added as part of
this patch and also not releated to this JIRA so don't want to change it as
part of this patch.
Gwen Shapira wrote:
Eric,
I noticed you have a lot of good comments on code style when reviewing
patches. I think we are all happy to improve code we are touching as part of
the patch, but we are also trying to keep the scope of patches mostly
contained and not touch unrelated code. This is so if we accidentally break
something it will be easier to figure out what happened.
Feel free to open new JIRA for those unrelated issues and submit patches.
Gwen
This is why I'm not marking these as issues in the review - they're just
comments. I'm hoping to plant the seed in peoples minds to think of Option not
in terms of Some and None, but instead to use the higher level APIs that Option
provides. I'm perfectly willing to open Jiras and file patches.
- Eric
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29912/#review72405
---
On Feb. 13, 2015, 12:46 a.m., Sriharsha Chintalapani wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29912/
---
(Updated Feb. 13, 2015, 12:46 a.m.)
Review request for kafka.
Bugs: KAFKA-1852
https://issues.apache.org/jira/browse/KAFKA-1852
Repository: kafka
Description
---
KAFKA-1852. OffsetCommitRequest can commit offset on unknown topic. Added
contains method to MetadataCache.
Diffs
-
core/src/main/scala/kafka/server/KafkaApis.scala
6ee7d8819a9ef923f3a65c865a0a3d8ded8845f0
core/src/main/scala/kafka/server/MetadataCache.scala
4c70aa7e0157b85de5e24736ebf487239c4571d0
core/src/main/scala/kafka/server/OffsetManager.scala
83d52643028c5628057dc0aa29819becfda61fdb
core/src/test/scala/unit/kafka/server/OffsetCommitTest.scala
5b93239cdc26b5be7696f4e7863adb9fbe5f0ed5
Diff: https://reviews.apache.org/r/29912/diff/
Testing
---
Thanks,
Sriharsha Chintalapani
Re: Review Request 29912: Patch for KAFKA-1852
On Feb. 13, 2015, 6:24 p.m., Eric Olander wrote:
core/src/main/scala/kafka/server/KafkaApis.scala, line 303
https://reviews.apache.org/r/29912/diff/3/?file=862697#file862697line303
This code could be done using map and getOrElse on the Option rather
than using pattern matching/reflection.
logManager.getLog(topicAndPartition).map{log = fetchOffsetsBefore(log,
timestamp, maxNumOffsets)
}.getOrElse {
if (timestamp == OffsetRequest.LatestTime || timestamp ==
OffsetRequest.EarliestTime)
Seq(0L)
else
Nil
}
Eric,
Thanks for the review. But the above code is not added as part of this
patch and also not releated to this JIRA so don't want to change it as part of
this patch.
- Sriharsha
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29912/#review72405
---
On Feb. 13, 2015, 12:46 a.m., Sriharsha Chintalapani wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29912/
---
(Updated Feb. 13, 2015, 12:46 a.m.)
Review request for kafka.
Bugs: KAFKA-1852
https://issues.apache.org/jira/browse/KAFKA-1852
Repository: kafka
Description
---
KAFKA-1852. OffsetCommitRequest can commit offset on unknown topic. Added
contains method to MetadataCache.
Diffs
-
core/src/main/scala/kafka/server/KafkaApis.scala
6ee7d8819a9ef923f3a65c865a0a3d8ded8845f0
core/src/main/scala/kafka/server/MetadataCache.scala
4c70aa7e0157b85de5e24736ebf487239c4571d0
core/src/main/scala/kafka/server/OffsetManager.scala
83d52643028c5628057dc0aa29819becfda61fdb
core/src/test/scala/unit/kafka/server/OffsetCommitTest.scala
5b93239cdc26b5be7696f4e7863adb9fbe5f0ed5
Diff: https://reviews.apache.org/r/29912/diff/
Testing
---
Thanks,
Sriharsha Chintalapani
Re: Review Request 29912: Patch for KAFKA-1852
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29912/#review72405
---
core/src/main/scala/kafka/server/KafkaApis.scala
https://reviews.apache.org/r/29912/#comment118464
This code could be done using map and getOrElse on the Option rather than
using pattern matching/reflection.
logManager.getLog(topicAndPartition).map{log = fetchOffsetsBefore(log,
timestamp, maxNumOffsets)
}.getOrElse {
if (timestamp == OffsetRequest.LatestTime || timestamp ==
OffsetRequest.EarliestTime)
Seq(0L)
else
Nil
}
- Eric Olander
On Feb. 13, 2015, 12:46 a.m., Sriharsha Chintalapani wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29912/
---
(Updated Feb. 13, 2015, 12:46 a.m.)
Review request for kafka.
Bugs: KAFKA-1852
https://issues.apache.org/jira/browse/KAFKA-1852
Repository: kafka
Description
---
KAFKA-1852. OffsetCommitRequest can commit offset on unknown topic. Added
contains method to MetadataCache.
Diffs
-
core/src/main/scala/kafka/server/KafkaApis.scala
6ee7d8819a9ef923f3a65c865a0a3d8ded8845f0
core/src/main/scala/kafka/server/MetadataCache.scala
4c70aa7e0157b85de5e24736ebf487239c4571d0
core/src/main/scala/kafka/server/OffsetManager.scala
83d52643028c5628057dc0aa29819becfda61fdb
core/src/test/scala/unit/kafka/server/OffsetCommitTest.scala
5b93239cdc26b5be7696f4e7863adb9fbe5f0ed5
Diff: https://reviews.apache.org/r/29912/diff/
Testing
---
Thanks,
Sriharsha Chintalapani
Re: Review Request 29912: Patch for KAFKA-1852
On Feb. 13, 2015, 6:24 p.m., Eric Olander wrote:
core/src/main/scala/kafka/server/KafkaApis.scala, line 303
https://reviews.apache.org/r/29912/diff/3/?file=862697#file862697line303
This code could be done using map and getOrElse on the Option rather
than using pattern matching/reflection.
logManager.getLog(topicAndPartition).map{log = fetchOffsetsBefore(log,
timestamp, maxNumOffsets)
}.getOrElse {
if (timestamp == OffsetRequest.LatestTime || timestamp ==
OffsetRequest.EarliestTime)
Seq(0L)
else
Nil
}
Sriharsha Chintalapani wrote:
Eric,
Thanks for the review. But the above code is not added as part of
this patch and also not releated to this JIRA so don't want to change it as
part of this patch.
Eric,
I noticed you have a lot of good comments on code style when reviewing patches.
I think we are all happy to improve code we are touching as part of the patch,
but we are also trying to keep the scope of patches mostly contained and not
touch unrelated code. This is so if we accidentally break something it will be
easier to figure out what happened.
Feel free to open new JIRA for those unrelated issues and submit patches.
Gwen
- Gwen
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29912/#review72405
---
On Feb. 13, 2015, 12:46 a.m., Sriharsha Chintalapani wrote:
---
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/29912/
---
(Updated Feb. 13, 2015, 12:46 a.m.)
Review request for kafka.
Bugs: KAFKA-1852
https://issues.apache.org/jira/browse/KAFKA-1852
Repository: kafka
Description
---
KAFKA-1852. OffsetCommitRequest can commit offset on unknown topic. Added
contains method to MetadataCache.
Diffs
-
core/src/main/scala/kafka/server/KafkaApis.scala
6ee7d8819a9ef923f3a65c865a0a3d8ded8845f0
core/src/main/scala/kafka/server/MetadataCache.scala
4c70aa7e0157b85de5e24736ebf487239c4571d0
core/src/main/scala/kafka/server/OffsetManager.scala
83d52643028c5628057dc0aa29819becfda61fdb
core/src/test/scala/unit/kafka/server/OffsetCommitTest.scala
5b93239cdc26b5be7696f4e7863adb9fbe5f0ed5
Diff: https://reviews.apache.org/r/29912/diff/
Testing
---
Thanks,
Sriharsha Chintalapani
Re: Review Request 29912: Patch for KAFKA-1852
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29912/#review72413 --- minor comment, looks good otherwise core/src/main/scala/kafka/server/KafkaApis.scala https://reviews.apache.org/r/29912/#comment118470 (Personally I also prefer the map+getOrElse idiom as it is way more concise. It does take getting used to - I use it but some prefer case matching) core/src/main/scala/kafka/server/OffsetManager.scala https://reviews.apache.org/r/29912/#comment118473 Minor comment. I think this may be better to pass in to the OffsetManager. We should even use it in loadOffsets to discard offsets that are from topics that have been deleted. We can do that in a separate jira - I don't think our handling for clearing out offsets on a delete topic is done yet - Onur Karaman did it for ZK based offsets but we need a separate jira to delete Kafka-based offsets. - Joel Koshy On Feb. 13, 2015, 12:46 a.m., Sriharsha Chintalapani wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29912/ --- (Updated Feb. 13, 2015, 12:46 a.m.) Review request for kafka. Bugs: KAFKA-1852 https://issues.apache.org/jira/browse/KAFKA-1852 Repository: kafka Description --- KAFKA-1852. OffsetCommitRequest can commit offset on unknown topic. Added contains method to MetadataCache. Diffs - core/src/main/scala/kafka/server/KafkaApis.scala 6ee7d8819a9ef923f3a65c865a0a3d8ded8845f0 core/src/main/scala/kafka/server/MetadataCache.scala 4c70aa7e0157b85de5e24736ebf487239c4571d0 core/src/main/scala/kafka/server/OffsetManager.scala 83d52643028c5628057dc0aa29819becfda61fdb core/src/test/scala/unit/kafka/server/OffsetCommitTest.scala 5b93239cdc26b5be7696f4e7863adb9fbe5f0ed5 Diff: https://reviews.apache.org/r/29912/diff/ Testing --- Thanks, Sriharsha Chintalapani
Re: Review Request 29912: Patch for KAFKA-1852
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29912/ --- (Updated Feb. 13, 2015, 12:46 a.m.) Review request for kafka. Bugs: KAFKA-1852 https://issues.apache.org/jira/browse/KAFKA-1852 Repository: kafka Description (updated) --- KAFKA-1852. OffsetCommitRequest can commit offset on unknown topic. Added contains method to MetadataCache. Diffs (updated) - core/src/main/scala/kafka/server/KafkaApis.scala 6ee7d8819a9ef923f3a65c865a0a3d8ded8845f0 core/src/main/scala/kafka/server/MetadataCache.scala 4c70aa7e0157b85de5e24736ebf487239c4571d0 core/src/main/scala/kafka/server/OffsetManager.scala 83d52643028c5628057dc0aa29819becfda61fdb core/src/test/scala/unit/kafka/server/OffsetCommitTest.scala 5b93239cdc26b5be7696f4e7863adb9fbe5f0ed5 Diff: https://reviews.apache.org/r/29912/diff/ Testing --- Thanks, Sriharsha Chintalapani
Re: Review Request 29912: Patch for KAFKA-1852
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29912/#review72166 --- core/src/main/scala/kafka/server/OffsetManager.scala https://reviews.apache.org/r/29912/#comment118171 Can we just add an exists(topic) method to metadataCache? That way we can just do something like offsetMetadata.groupBy((topicPartition, offsetMetadata) = metadataCache.contains(topicPartition.topic)) - Joel Koshy On Jan. 19, 2015, 6:44 p.m., Sriharsha Chintalapani wrote: --- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29912/ --- (Updated Jan. 19, 2015, 6:44 p.m.) Review request for kafka. Bugs: KAFKA-1852 https://issues.apache.org/jira/browse/KAFKA-1852 Repository: kafka Description --- KAFKA-1852. OffsetCommitRequest can commit offset on unknown topic. Diffs - core/src/main/scala/kafka/server/KafkaApis.scala ec8d9f7ba44741db40875458bd524c4062ad6a26 core/src/main/scala/kafka/server/OffsetManager.scala 0bdd42fea931cddd072c0fff765b10526db6840a core/src/test/scala/unit/kafka/server/OffsetCommitTest.scala 5b93239cdc26b5be7696f4e7863adb9fbe5f0ed5 Diff: https://reviews.apache.org/r/29912/diff/ Testing --- Thanks, Sriharsha Chintalapani
Re: Review Request 29912: Patch for KAFKA-1852
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29912/ --- (Updated Jan. 19, 2015, 6:44 p.m.) Review request for kafka. Bugs: KAFKA-1852 https://issues.apache.org/jira/browse/KAFKA-1852 Repository: kafka Description --- KAFKA-1852. OffsetCommitRequest can commit offset on unknown topic. Diffs (updated) - core/src/main/scala/kafka/server/KafkaApis.scala ec8d9f7ba44741db40875458bd524c4062ad6a26 core/src/main/scala/kafka/server/OffsetManager.scala 0bdd42fea931cddd072c0fff765b10526db6840a core/src/test/scala/unit/kafka/server/OffsetCommitTest.scala 5b93239cdc26b5be7696f4e7863adb9fbe5f0ed5 Diff: https://reviews.apache.org/r/29912/diff/ Testing --- Thanks, Sriharsha Chintalapani
Review Request 29912: Patch for KAFKA-1852
--- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29912/ --- Review request for kafka. Bugs: KAFKA-1852 https://issues.apache.org/jira/browse/KAFKA-1852 Repository: kafka Description --- KAFKA-1852. OffsetCommitRequest can commit offset on unknown topic. Diffs - core/src/main/scala/kafka/server/KafkaApis.scala c011a1b79bd6c4e832fe7d097daacb0d647d1cd4 core/src/main/scala/kafka/server/OffsetManager.scala 3c79428962604800983415f6f705e04f52acb8fb core/src/test/scala/unit/kafka/server/OffsetCommitTest.scala 4a3a5b264a021e55c39f4d7424ce04ee591503ef Diff: https://reviews.apache.org/r/29912/diff/ Testing --- Thanks, Sriharsha Chintalapani
