Re: Review Request 29912: Patch for KAFKA-1852

[Joel Koshy]

---
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

[Sriharsha Chintalapani]

> 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

[Sriharsha Chintalapani]

---
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

[Joel Koshy]

---
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


This can just be a val



core/src/main/scala/kafka/server/MetadataCache.scala


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

[Sriharsha Chintalapani]

> On Feb. 13, 2015, 7:01 p.m., Joel Koshy wrote:
> > core/src/main/scala/kafka/server/OffsetManager.scala, line 215
> > 
> >
> > 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

[Sriharsha Chintalapani]

---
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

[Joel Koshy]

> On Feb. 13, 2015, 7:01 p.m., Joel Koshy wrote:
> > core/src/main/scala/kafka/server/OffsetManager.scala, line 215
> > 
> >
> > 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

[Sriharsha Chintalapani]

> On Feb. 13, 2015, 7:01 p.m., Joel Koshy wrote:
> > core/src/main/scala/kafka/server/OffsetManager.scala, line 215
> > 
> >
> > 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

[Sriharsha Chintalapani]

---
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 (updated)
---

KAFKA-1852. OffsetCommitRequest can commit offset on unknown topic. Added 
contains method to MetadataCache.


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/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

[Eric Olander]

> On Feb. 13, 2015, 6:24 p.m., Eric Olander wrote:
> > core/src/main/scala/kafka/server/KafkaApis.scala, line 303
> > 
> >
> > 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

[Joel Koshy]

---
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


(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


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

[Gwen Shapira]

> On Feb. 13, 2015, 6:24 p.m., Eric Olander wrote:
> > core/src/main/scala/kafka/server/KafkaApis.scala, line 303
> > 
> >
> > 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

[Sriharsha Chintalapani]

> On Feb. 13, 2015, 6:24 p.m., Eric Olander wrote:
> > core/src/main/scala/kafka/server/KafkaApis.scala, line 303
> > 
> >
> > 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

[Eric Olander]

---
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


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

[Sriharsha Chintalapani]

---
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

[Joel Koshy]

---
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


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

[Sriharsha Chintalapani]

---
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

[Sriharsha Chintalapani]

---
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