You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@kafka.apache.org by Sriharsha Chintalapani <ha...@hortonworks.com> on 2015/01/15 02:51:55 UTC

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

Re: Review Request 29912: Patch for KAFKA-1852

Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
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

Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
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

Posted by Sriharsha Chintalapani <ha...@hortonworks.com>.
-----------------------------------------------------------
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

Posted by Sriharsha Chintalapani <ha...@hortonworks.com>.

> 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

Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
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

Posted by Sriharsha Chintalapani <ha...@hortonworks.com>.
-----------------------------------------------------------
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

Posted by Sriharsha Chintalapani <ha...@hortonworks.com>.
-----------------------------------------------------------
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

Posted by Joel Koshy <jj...@gmail.com>.

> 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

Posted by Sriharsha Chintalapani <ha...@hortonworks.com>.

> 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

Posted by Sriharsha Chintalapani <ha...@hortonworks.com>.

> 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

Posted by Joel Koshy <jj...@gmail.com>.
-----------------------------------------------------------
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

Posted by Gwen Shapira <gs...@cloudera.com>.

> 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

Posted by Sriharsha Chintalapani <ha...@hortonworks.com>.

> 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

Posted by Eric Olander <ol...@gmail.com>.

> 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

Posted by Eric Olander <ol...@gmail.com>.
-----------------------------------------------------------
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

Posted by Sriharsha Chintalapani <ha...@hortonworks.com>.
-----------------------------------------------------------
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

Posted by Sriharsha Chintalapani <ha...@hortonworks.com>.
-----------------------------------------------------------
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