You are viewing a plain text version of this content. The canonical link for it is here.

Posted to dev@kafka.apache.org by Gwen Shapira <gs...@cloudera.com> on 2014/09/22 07:57:26 UTC

Review Request 25886: KAFKA-1555: provide strong consistency with reasonable availability

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25886/
-----------------------------------------------------------

Review request for kafka.


Repository: kafka


Description
-------

KAFKA-1555: provide strong consistency with reasonable availability


Diffs
-----

  core/src/main/scala/kafka/cluster/Partition.scala ff106b4 
  core/src/main/scala/kafka/common/ErrorMapping.scala 3fae791 
  core/src/main/scala/kafka/log/LogConfig.scala 5746ad4 

Diff: https://reviews.apache.org/r/25886/diff/


Testing
-------

With 3 broker cluster, created 3 topics each with 1 partition and 3 replicas, with 1,3 and 4 min.insync.replicas.
* min.insync.replicas=1 behaved normally (all writes succeeded as long as a broker was up)
* min.insync.replicas=3 returned NotEnoughReplicas when required.acks=-1 and one broker was down
* min.insync.replicas=4 returned NotEnoughReplicas when required.acks=-1

See notes about retry behavior in the JIRA.


Thanks,

Gwen Shapira

Re: Review Request 25886: KAFKA-1555: provide strong consistency with reasonable availability

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


> On Sept. 24, 2014, 5:06 a.m., Jun Rao wrote:
> > core/src/main/scala/kafka/cluster/Partition.scala, lines 272-276
> > <https://reviews.apache.org/r/25886/diff/4/?file=700999#file700999line272>
> >
> >     Perhaps we should just do leaderReplica.log.get.config.minInSyncReplicas.

Thank you! I knew this looked too complex, but wasn't sure what to do with an Option.


- Gwen


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25886/#review54386
-----------------------------------------------------------


On Sept. 23, 2014, 12:28 a.m., Gwen Shapira wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25886/
> -----------------------------------------------------------
> 
> (Updated Sept. 23, 2014, 12:28 a.m.)
> 
> 
> Review request for kafka.
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> KAFKA-1555: provide strong consistency with reasonable availability
> 
> 
> Diffs
> -----
> 
>   clients/src/main/java/org/apache/kafka/common/errors/NotEnoughReplicasException.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/protocol/Errors.java d434f42 
>   core/src/main/scala/kafka/cluster/Partition.scala ff106b4 
>   core/src/main/scala/kafka/common/ErrorMapping.scala 3fae791 
>   core/src/main/scala/kafka/common/NotEnoughReplicasException.scala PRE-CREATION 
>   core/src/main/scala/kafka/log/LogConfig.scala 5746ad4 
>   core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala 39f777b 
>   core/src/test/scala/unit/kafka/producer/SyncProducerTest.scala 24deea0 
>   core/src/test/scala/unit/kafka/utils/TestUtils.scala 2dbdd3c 
> 
> Diff: https://reviews.apache.org/r/25886/diff/
> 
> 
> Testing
> -------
> 
> With 3 broker cluster, created 3 topics each with 1 partition and 3 replicas, with 1,3 and 4 min.insync.replicas.
> * min.insync.replicas=1 behaved normally (all writes succeeded as long as a broker was up)
> * min.insync.replicas=3 returned NotEnoughReplicas when required.acks=-1 and one broker was down
> * min.insync.replicas=4 returned NotEnoughReplicas when required.acks=-1
> 
> See notes about retry behavior in the JIRA.
> 
> 
> Thanks,
> 
> Gwen Shapira
> 
>

Re: Review Request 25886: KAFKA-1555: provide strong consistency with reasonable availability

Posted by Jun Rao <ju...@gmail.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25886/#review54386
-----------------------------------------------------------


Thanks for the patch. Looks good. A few comments below.


clients/src/main/java/org/apache/kafka/common/errors/NotEnoughReplicasException.java
<https://reviews.apache.org/r/25886/#comment94521>

    Arguably, this is a retriable exception since the ISR size changes over time.



core/src/main/scala/kafka/cluster/Partition.scala
<https://reviews.apache.org/r/25886/#comment94522>

    Perhaps we should just do leaderReplica.log.get.config.minInSyncReplicas.



core/src/main/scala/kafka/log/LogConfig.scala
<https://reviews.apache.org/r/25886/#comment94523>

    Lower case Topic?



core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala
<https://reviews.apache.org/r/25886/#comment94524>

    Could we try/catch the exception and check the cause is NotEnoughReplicas?



core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala
<https://reviews.apache.org/r/25886/#comment94525>

    Ditto as the above.


- Jun Rao


On Sept. 23, 2014, 12:28 a.m., Gwen Shapira wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25886/
> -----------------------------------------------------------
> 
> (Updated Sept. 23, 2014, 12:28 a.m.)
> 
> 
> Review request for kafka.
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> KAFKA-1555: provide strong consistency with reasonable availability
> 
> 
> Diffs
> -----
> 
>   clients/src/main/java/org/apache/kafka/common/errors/NotEnoughReplicasException.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/protocol/Errors.java d434f42 
>   core/src/main/scala/kafka/cluster/Partition.scala ff106b4 
>   core/src/main/scala/kafka/common/ErrorMapping.scala 3fae791 
>   core/src/main/scala/kafka/common/NotEnoughReplicasException.scala PRE-CREATION 
>   core/src/main/scala/kafka/log/LogConfig.scala 5746ad4 
>   core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala 39f777b 
>   core/src/test/scala/unit/kafka/producer/SyncProducerTest.scala 24deea0 
>   core/src/test/scala/unit/kafka/utils/TestUtils.scala 2dbdd3c 
> 
> Diff: https://reviews.apache.org/r/25886/diff/
> 
> 
> Testing
> -------
> 
> With 3 broker cluster, created 3 topics each with 1 partition and 3 replicas, with 1,3 and 4 min.insync.replicas.
> * min.insync.replicas=1 behaved normally (all writes succeeded as long as a broker was up)
> * min.insync.replicas=3 returned NotEnoughReplicas when required.acks=-1 and one broker was down
> * min.insync.replicas=4 returned NotEnoughReplicas when required.acks=-1
> 
> See notes about retry behavior in the JIRA.
> 
> 
> Thanks,
> 
> Gwen Shapira
> 
>

Re: Review Request 25886: KAFKA-1555: provide strong consistency with reasonable availability

Posted by Jun Rao <ju...@gmail.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25886/#review55988
-----------------------------------------------------------


Thanks for the patch. Looks good to me. Could you rebase to latest trunk?

- Jun Rao


On Oct. 8, 2014, 1:46 a.m., Gwen Shapira wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25886/
> -----------------------------------------------------------
> 
> (Updated Oct. 8, 2014, 1:46 a.m.)
> 
> 
> Review request for kafka.
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> KAFKA-1555: provide strong consistency with reasonable availability
> 
> 
> Diffs
> -----
> 
>   clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java f9de4af 
>   clients/src/main/java/org/apache/kafka/common/config/ConfigDef.java addc906 
>   clients/src/main/java/org/apache/kafka/common/errors/NotEnoughReplicasAfterAppendException.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/errors/NotEnoughReplicasException.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/protocol/Errors.java d434f42 
>   core/src/main/scala/kafka/cluster/Partition.scala ff106b4 
>   core/src/main/scala/kafka/common/ErrorMapping.scala 3fae791 
>   core/src/main/scala/kafka/common/NotEnoughReplicasAfterAppendException.scala PRE-CREATION 
>   core/src/main/scala/kafka/common/NotEnoughReplicasException.scala PRE-CREATION 
>   core/src/main/scala/kafka/log/LogConfig.scala 5746ad4 
>   core/src/main/scala/kafka/producer/SyncProducerConfig.scala 69b2d0c 
>   core/src/main/scala/kafka/server/KafkaApis.scala c584b55 
>   core/src/main/scala/kafka/server/KafkaConfig.scala 165c816 
>   core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala 39f777b 
>   core/src/test/scala/unit/kafka/producer/ProducerTest.scala dd71d81 
>   core/src/test/scala/unit/kafka/producer/SyncProducerTest.scala 24deea0 
>   core/src/test/scala/unit/kafka/utils/TestUtils.scala 2dbdd3c 
> 
> Diff: https://reviews.apache.org/r/25886/diff/
> 
> 
> Testing
> -------
> 
> With 3 broker cluster, created 3 topics each with 1 partition and 3 replicas, with 1,3 and 4 min.insync.replicas.
> * min.insync.replicas=1 behaved normally (all writes succeeded as long as a broker was up)
> * min.insync.replicas=3 returned NotEnoughReplicas when required.acks=-1 and one broker was down
> * min.insync.replicas=4 returned NotEnoughReplicas when required.acks=-1
> 
> See notes about retry behavior in the JIRA.
> 
> 
> Thanks,
> 
> Gwen Shapira
> 
>

Re: Review Request 25886: KAFKA-1555: provide strong consistency with reasonable availability

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


> On Oct. 8, 2014, 2:04 a.m., Joel Koshy wrote:
> > clients/src/main/java/org/apache/kafka/common/config/ConfigDef.java, line 271
> > <https://reviews.apache.org/r/25886/diff/8/?file=714236#file714236line271>
> >
> >     Where is this used?

Used in ProducerConfig line 180:
 .define(ACKS_CONFIG,
                                        Type.STRING,
                                        "1",
                                        in(Arrays.asList("all","-1", "0", "1")),
                                        Importance.HIGH,
                                        ACKS_DOC)
                                        
ACKS are defined as a String and not a Short. I don't want to change a public API, and there are discussions of turning it into an enum anyway, so validating the input against a list of strings seems appropriate.


- Gwen


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25886/#review55745
-----------------------------------------------------------


On Oct. 8, 2014, 1:46 a.m., Gwen Shapira wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25886/
> -----------------------------------------------------------
> 
> (Updated Oct. 8, 2014, 1:46 a.m.)
> 
> 
> Review request for kafka.
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> KAFKA-1555: provide strong consistency with reasonable availability
> 
> 
> Diffs
> -----
> 
>   clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java f9de4af 
>   clients/src/main/java/org/apache/kafka/common/config/ConfigDef.java addc906 
>   clients/src/main/java/org/apache/kafka/common/errors/NotEnoughReplicasAfterAppendException.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/errors/NotEnoughReplicasException.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/protocol/Errors.java d434f42 
>   core/src/main/scala/kafka/cluster/Partition.scala ff106b4 
>   core/src/main/scala/kafka/common/ErrorMapping.scala 3fae791 
>   core/src/main/scala/kafka/common/NotEnoughReplicasAfterAppendException.scala PRE-CREATION 
>   core/src/main/scala/kafka/common/NotEnoughReplicasException.scala PRE-CREATION 
>   core/src/main/scala/kafka/log/LogConfig.scala 5746ad4 
>   core/src/main/scala/kafka/producer/SyncProducerConfig.scala 69b2d0c 
>   core/src/main/scala/kafka/server/KafkaApis.scala c584b55 
>   core/src/main/scala/kafka/server/KafkaConfig.scala 165c816 
>   core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala 39f777b 
>   core/src/test/scala/unit/kafka/producer/ProducerTest.scala dd71d81 
>   core/src/test/scala/unit/kafka/producer/SyncProducerTest.scala 24deea0 
>   core/src/test/scala/unit/kafka/utils/TestUtils.scala 2dbdd3c 
> 
> Diff: https://reviews.apache.org/r/25886/diff/
> 
> 
> Testing
> -------
> 
> With 3 broker cluster, created 3 topics each with 1 partition and 3 replicas, with 1,3 and 4 min.insync.replicas.
> * min.insync.replicas=1 behaved normally (all writes succeeded as long as a broker was up)
> * min.insync.replicas=3 returned NotEnoughReplicas when required.acks=-1 and one broker was down
> * min.insync.replicas=4 returned NotEnoughReplicas when required.acks=-1
> 
> See notes about retry behavior in the JIRA.
> 
> 
> Thanks,
> 
> Gwen Shapira
> 
>

Re: Review Request 25886: KAFKA-1555: provide strong consistency with reasonable availability

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

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25886/#review55745
-----------------------------------------------------------



clients/src/main/java/org/apache/kafka/common/config/ConfigDef.java
<https://reviews.apache.org/r/25886/#comment96135>

    Where is this used?



core/src/main/scala/kafka/cluster/Partition.scala
<https://reviews.apache.org/r/25886/#comment96138>

    may be worth clarifying the comment - i.e., in this particular scenario the request was already appended locally and then added to the purgatory before the ISR was shrunk.



core/src/main/scala/kafka/log/LogConfig.scala
<https://reviews.apache.org/r/25886/#comment96149>

    We can validate though at the time the topic is created or altered right? i.e., in the admin utils although that does move a portion of the log config validation outside.


- Joel Koshy


On Oct. 8, 2014, 1:46 a.m., Gwen Shapira wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25886/
> -----------------------------------------------------------
> 
> (Updated Oct. 8, 2014, 1:46 a.m.)
> 
> 
> Review request for kafka.
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> KAFKA-1555: provide strong consistency with reasonable availability
> 
> 
> Diffs
> -----
> 
>   clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java f9de4af 
>   clients/src/main/java/org/apache/kafka/common/config/ConfigDef.java addc906 
>   clients/src/main/java/org/apache/kafka/common/errors/NotEnoughReplicasAfterAppendException.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/errors/NotEnoughReplicasException.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/protocol/Errors.java d434f42 
>   core/src/main/scala/kafka/cluster/Partition.scala ff106b4 
>   core/src/main/scala/kafka/common/ErrorMapping.scala 3fae791 
>   core/src/main/scala/kafka/common/NotEnoughReplicasAfterAppendException.scala PRE-CREATION 
>   core/src/main/scala/kafka/common/NotEnoughReplicasException.scala PRE-CREATION 
>   core/src/main/scala/kafka/log/LogConfig.scala 5746ad4 
>   core/src/main/scala/kafka/producer/SyncProducerConfig.scala 69b2d0c 
>   core/src/main/scala/kafka/server/KafkaApis.scala c584b55 
>   core/src/main/scala/kafka/server/KafkaConfig.scala 165c816 
>   core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala 39f777b 
>   core/src/test/scala/unit/kafka/producer/ProducerTest.scala dd71d81 
>   core/src/test/scala/unit/kafka/producer/SyncProducerTest.scala 24deea0 
>   core/src/test/scala/unit/kafka/utils/TestUtils.scala 2dbdd3c 
> 
> Diff: https://reviews.apache.org/r/25886/diff/
> 
> 
> Testing
> -------
> 
> With 3 broker cluster, created 3 topics each with 1 partition and 3 replicas, with 1,3 and 4 min.insync.replicas.
> * min.insync.replicas=1 behaved normally (all writes succeeded as long as a broker was up)
> * min.insync.replicas=3 returned NotEnoughReplicas when required.acks=-1 and one broker was down
> * min.insync.replicas=4 returned NotEnoughReplicas when required.acks=-1
> 
> See notes about retry behavior in the JIRA.
> 
> 
> Thanks,
> 
> Gwen Shapira
> 
>

Re: Review Request 25886: KAFKA-1555: provide strong consistency with reasonable availability

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

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25886/
-----------------------------------------------------------

(Updated Oct. 9, 2014, 11:21 p.m.)


Review request for kafka.


Changes
-------

The correct test is back. Not sure how the re-base could have changed that, but I should have double checked the diff. Thanks!


Repository: kafka


Description
-------

KAFKA-1555: provide strong consistency with reasonable availability


Diffs (updated)
-----

  clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java f9de4af 
  clients/src/main/java/org/apache/kafka/common/config/ConfigDef.java addc906 
  clients/src/main/java/org/apache/kafka/common/errors/NotEnoughReplicasAfterAppendException.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/errors/NotEnoughReplicasException.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/protocol/Errors.java d5f5de3 
  core/src/main/scala/kafka/cluster/Partition.scala ff106b4 
  core/src/main/scala/kafka/common/ErrorMapping.scala a190607 
  core/src/main/scala/kafka/common/NotEnoughReplicasAfterAppendException.scala PRE-CREATION 
  core/src/main/scala/kafka/common/NotEnoughReplicasException.scala PRE-CREATION 
  core/src/main/scala/kafka/log/LogConfig.scala 5746ad4 
  core/src/main/scala/kafka/producer/SyncProducerConfig.scala 69b2d0c 
  core/src/main/scala/kafka/server/KafkaApis.scala c584b55 
  core/src/main/scala/kafka/server/KafkaConfig.scala 165c816 
  core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala 39f777b 
  core/src/test/scala/unit/kafka/producer/ProducerTest.scala dd71d81 
  core/src/test/scala/unit/kafka/producer/SyncProducerTest.scala 24deea0 
  core/src/test/scala/unit/kafka/utils/TestUtils.scala 2dbdd3c 

Diff: https://reviews.apache.org/r/25886/diff/


Testing
-------

With 3 broker cluster, created 3 topics each with 1 partition and 3 replicas, with 1,3 and 4 min.insync.replicas.
* min.insync.replicas=1 behaved normally (all writes succeeded as long as a broker was up)
* min.insync.replicas=3 returned NotEnoughReplicas when required.acks=-1 and one broker was down
* min.insync.replicas=4 returned NotEnoughReplicas when required.acks=-1

See notes about retry behavior in the JIRA.


Thanks,

Gwen Shapira

Re: Review Request 25886: KAFKA-1555: provide strong consistency with reasonable availability

Posted by Jun Rao <ju...@gmail.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25886/#review56072
-----------------------------------------------------------



core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala
<https://reviews.apache.org/r/25886/#comment96425>

    I thought we want to catch the exception and check the cause like below. Was this reverted inadvertently?


- Jun Rao


On Oct. 9, 2014, 9:58 p.m., Gwen Shapira wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25886/
> -----------------------------------------------------------
> 
> (Updated Oct. 9, 2014, 9:58 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> KAFKA-1555: provide strong consistency with reasonable availability
> 
> 
> Diffs
> -----
> 
>   clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java f9de4af 
>   clients/src/main/java/org/apache/kafka/common/config/ConfigDef.java addc906 
>   clients/src/main/java/org/apache/kafka/common/errors/NotEnoughReplicasAfterAppendException.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/errors/NotEnoughReplicasException.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/protocol/Errors.java d5f5de3 
>   core/src/main/scala/kafka/cluster/Partition.scala ff106b4 
>   core/src/main/scala/kafka/common/ErrorMapping.scala a190607 
>   core/src/main/scala/kafka/common/NotEnoughReplicasAfterAppendException.scala PRE-CREATION 
>   core/src/main/scala/kafka/common/NotEnoughReplicasException.scala PRE-CREATION 
>   core/src/main/scala/kafka/log/LogConfig.scala 5746ad4 
>   core/src/main/scala/kafka/producer/SyncProducerConfig.scala 69b2d0c 
>   core/src/main/scala/kafka/server/KafkaApis.scala c584b55 
>   core/src/main/scala/kafka/server/KafkaConfig.scala 165c816 
>   core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala 39f777b 
>   core/src/test/scala/unit/kafka/producer/ProducerTest.scala dd71d81 
>   core/src/test/scala/unit/kafka/producer/SyncProducerTest.scala 24deea0 
>   core/src/test/scala/unit/kafka/utils/TestUtils.scala 2dbdd3c 
> 
> Diff: https://reviews.apache.org/r/25886/diff/
> 
> 
> Testing
> -------
> 
> With 3 broker cluster, created 3 topics each with 1 partition and 3 replicas, with 1,3 and 4 min.insync.replicas.
> * min.insync.replicas=1 behaved normally (all writes succeeded as long as a broker was up)
> * min.insync.replicas=3 returned NotEnoughReplicas when required.acks=-1 and one broker was down
> * min.insync.replicas=4 returned NotEnoughReplicas when required.acks=-1
> 
> See notes about retry behavior in the JIRA.
> 
> 
> Thanks,
> 
> Gwen Shapira
> 
>

Re: Review Request 25886: KAFKA-1555: provide strong consistency with reasonable availability

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

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25886/
-----------------------------------------------------------

(Updated Oct. 9, 2014, 9:58 p.m.)


Review request for kafka.


Changes
-------

rebased on trunk


Repository: kafka


Description
-------

KAFKA-1555: provide strong consistency with reasonable availability


Diffs (updated)
-----

  clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java f9de4af 
  clients/src/main/java/org/apache/kafka/common/config/ConfigDef.java addc906 
  clients/src/main/java/org/apache/kafka/common/errors/NotEnoughReplicasAfterAppendException.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/errors/NotEnoughReplicasException.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/protocol/Errors.java d5f5de3 
  core/src/main/scala/kafka/cluster/Partition.scala ff106b4 
  core/src/main/scala/kafka/common/ErrorMapping.scala a190607 
  core/src/main/scala/kafka/common/NotEnoughReplicasAfterAppendException.scala PRE-CREATION 
  core/src/main/scala/kafka/common/NotEnoughReplicasException.scala PRE-CREATION 
  core/src/main/scala/kafka/log/LogConfig.scala 5746ad4 
  core/src/main/scala/kafka/producer/SyncProducerConfig.scala 69b2d0c 
  core/src/main/scala/kafka/server/KafkaApis.scala c584b55 
  core/src/main/scala/kafka/server/KafkaConfig.scala 165c816 
  core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala 39f777b 
  core/src/test/scala/unit/kafka/producer/ProducerTest.scala dd71d81 
  core/src/test/scala/unit/kafka/producer/SyncProducerTest.scala 24deea0 
  core/src/test/scala/unit/kafka/utils/TestUtils.scala 2dbdd3c 

Diff: https://reviews.apache.org/r/25886/diff/


Testing
-------

With 3 broker cluster, created 3 topics each with 1 partition and 3 replicas, with 1,3 and 4 min.insync.replicas.
* min.insync.replicas=1 behaved normally (all writes succeeded as long as a broker was up)
* min.insync.replicas=3 returned NotEnoughReplicas when required.acks=-1 and one broker was down
* min.insync.replicas=4 returned NotEnoughReplicas when required.acks=-1

See notes about retry behavior in the JIRA.


Thanks,

Gwen Shapira

Re: Review Request 25886: KAFKA-1555: provide strong consistency with reasonable availability

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

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25886/
-----------------------------------------------------------

(Updated Oct. 8, 2014, 1:46 a.m.)


Review request for kafka.


Changes
-------

Minor fixes based on Jun's feedback.


Repository: kafka


Description
-------

KAFKA-1555: provide strong consistency with reasonable availability


Diffs (updated)
-----

  clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java f9de4af 
  clients/src/main/java/org/apache/kafka/common/config/ConfigDef.java addc906 
  clients/src/main/java/org/apache/kafka/common/errors/NotEnoughReplicasAfterAppendException.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/errors/NotEnoughReplicasException.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/protocol/Errors.java d434f42 
  core/src/main/scala/kafka/cluster/Partition.scala ff106b4 
  core/src/main/scala/kafka/common/ErrorMapping.scala 3fae791 
  core/src/main/scala/kafka/common/NotEnoughReplicasAfterAppendException.scala PRE-CREATION 
  core/src/main/scala/kafka/common/NotEnoughReplicasException.scala PRE-CREATION 
  core/src/main/scala/kafka/log/LogConfig.scala 5746ad4 
  core/src/main/scala/kafka/producer/SyncProducerConfig.scala 69b2d0c 
  core/src/main/scala/kafka/server/KafkaApis.scala c584b55 
  core/src/main/scala/kafka/server/KafkaConfig.scala 165c816 
  core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala 39f777b 
  core/src/test/scala/unit/kafka/producer/ProducerTest.scala dd71d81 
  core/src/test/scala/unit/kafka/producer/SyncProducerTest.scala 24deea0 
  core/src/test/scala/unit/kafka/utils/TestUtils.scala 2dbdd3c 

Diff: https://reviews.apache.org/r/25886/diff/


Testing
-------

With 3 broker cluster, created 3 topics each with 1 partition and 3 replicas, with 1,3 and 4 min.insync.replicas.
* min.insync.replicas=1 behaved normally (all writes succeeded as long as a broker was up)
* min.insync.replicas=3 returned NotEnoughReplicas when required.acks=-1 and one broker was down
* min.insync.replicas=4 returned NotEnoughReplicas when required.acks=-1

See notes about retry behavior in the JIRA.


Thanks,

Gwen Shapira

Re: Review Request 25886: KAFKA-1555: provide strong consistency with reasonable availability

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


> On Oct. 8, 2014, midnight, Jun Rao wrote:
> > core/src/main/scala/kafka/cluster/Partition.scala, line 287
> > <https://reviews.apache.org/r/25886/diff/8/?file=714240#file714240line287>
> >
> >     The error code should be NotEnoughReplicasAfterAppendCode.

Good catch.


- Gwen


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25886/#review55730
-----------------------------------------------------------


On Oct. 8, 2014, 1:46 a.m., Gwen Shapira wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25886/
> -----------------------------------------------------------
> 
> (Updated Oct. 8, 2014, 1:46 a.m.)
> 
> 
> Review request for kafka.
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> KAFKA-1555: provide strong consistency with reasonable availability
> 
> 
> Diffs
> -----
> 
>   clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java f9de4af 
>   clients/src/main/java/org/apache/kafka/common/config/ConfigDef.java addc906 
>   clients/src/main/java/org/apache/kafka/common/errors/NotEnoughReplicasAfterAppendException.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/errors/NotEnoughReplicasException.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/protocol/Errors.java d434f42 
>   core/src/main/scala/kafka/cluster/Partition.scala ff106b4 
>   core/src/main/scala/kafka/common/ErrorMapping.scala 3fae791 
>   core/src/main/scala/kafka/common/NotEnoughReplicasAfterAppendException.scala PRE-CREATION 
>   core/src/main/scala/kafka/common/NotEnoughReplicasException.scala PRE-CREATION 
>   core/src/main/scala/kafka/log/LogConfig.scala 5746ad4 
>   core/src/main/scala/kafka/producer/SyncProducerConfig.scala 69b2d0c 
>   core/src/main/scala/kafka/server/KafkaApis.scala c584b55 
>   core/src/main/scala/kafka/server/KafkaConfig.scala 165c816 
>   core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala 39f777b 
>   core/src/test/scala/unit/kafka/producer/ProducerTest.scala dd71d81 
>   core/src/test/scala/unit/kafka/producer/SyncProducerTest.scala 24deea0 
>   core/src/test/scala/unit/kafka/utils/TestUtils.scala 2dbdd3c 
> 
> Diff: https://reviews.apache.org/r/25886/diff/
> 
> 
> Testing
> -------
> 
> With 3 broker cluster, created 3 topics each with 1 partition and 3 replicas, with 1,3 and 4 min.insync.replicas.
> * min.insync.replicas=1 behaved normally (all writes succeeded as long as a broker was up)
> * min.insync.replicas=3 returned NotEnoughReplicas when required.acks=-1 and one broker was down
> * min.insync.replicas=4 returned NotEnoughReplicas when required.acks=-1
> 
> See notes about retry behavior in the JIRA.
> 
> 
> Thanks,
> 
> Gwen Shapira
> 
>

Re: Review Request 25886: KAFKA-1555: provide strong consistency with reasonable availability

Posted by Jun Rao <ju...@gmail.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25886/#review55730
-----------------------------------------------------------


Looks good. Just some minor comments below.


clients/src/main/java/org/apache/kafka/common/protocol/Errors.java
<https://reviews.apache.org/r/25886/#comment96119>

    Perhaps the message can be "Messages are rejected since there are not enough in-sync replicas than required.".



clients/src/main/java/org/apache/kafka/common/protocol/Errors.java
<https://reviews.apache.org/r/25886/#comment96120>

    How about "Messages are written to the log, but to fewer in-sync replicas than required."?



core/src/main/scala/kafka/cluster/Partition.scala
<https://reviews.apache.org/r/25886/#comment96123>

    The error code should be NotEnoughReplicasAfterAppendCode.



core/src/main/scala/kafka/common/NotEnoughReplicasException.scala
<https://reviews.apache.org/r/25886/#comment96124>

    Probably add that messages are rejected.



core/src/main/scala/kafka/log/LogConfig.scala
<https://reviews.apache.org/r/25886/#comment96125>

    -1 (or all)



core/src/main/scala/kafka/server/KafkaConfig.scala
<https://reviews.apache.org/r/25886/#comment96128>

    -1 (all)


- Jun Rao


On Oct. 6, 2014, 8:28 p.m., Gwen Shapira wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25886/
> -----------------------------------------------------------
> 
> (Updated Oct. 6, 2014, 8:28 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> KAFKA-1555: provide strong consistency with reasonable availability
> 
> 
> Diffs
> -----
> 
>   clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java f9de4af 
>   clients/src/main/java/org/apache/kafka/common/config/ConfigDef.java addc906 
>   clients/src/main/java/org/apache/kafka/common/errors/NotEnoughReplicasAfterAppendException.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/errors/NotEnoughReplicasException.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/protocol/Errors.java d434f42 
>   core/src/main/scala/kafka/cluster/Partition.scala ff106b4 
>   core/src/main/scala/kafka/common/ErrorMapping.scala 3fae791 
>   core/src/main/scala/kafka/common/NotEnoughReplicasAfterAppendException.scala PRE-CREATION 
>   core/src/main/scala/kafka/common/NotEnoughReplicasException.scala PRE-CREATION 
>   core/src/main/scala/kafka/log/LogConfig.scala 5746ad4 
>   core/src/main/scala/kafka/producer/SyncProducerConfig.scala 69b2d0c 
>   core/src/main/scala/kafka/server/KafkaApis.scala c584b55 
>   core/src/main/scala/kafka/server/KafkaConfig.scala 165c816 
>   core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala 39f777b 
>   core/src/test/scala/unit/kafka/producer/ProducerTest.scala dd71d81 
>   core/src/test/scala/unit/kafka/producer/SyncProducerTest.scala 24deea0 
>   core/src/test/scala/unit/kafka/utils/TestUtils.scala 2dbdd3c 
> 
> Diff: https://reviews.apache.org/r/25886/diff/
> 
> 
> Testing
> -------
> 
> With 3 broker cluster, created 3 topics each with 1 partition and 3 replicas, with 1,3 and 4 min.insync.replicas.
> * min.insync.replicas=1 behaved normally (all writes succeeded as long as a broker was up)
> * min.insync.replicas=3 returned NotEnoughReplicas when required.acks=-1 and one broker was down
> * min.insync.replicas=4 returned NotEnoughReplicas when required.acks=-1
> 
> See notes about retry behavior in the JIRA.
> 
> 
> Thanks,
> 
> Gwen Shapira
> 
>

Re: Review Request 25886: KAFKA-1555: provide strong consistency with reasonable availability

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

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25886/
-----------------------------------------------------------

(Updated Oct. 6, 2014, 8:28 p.m.)


Review request for kafka.


Changes
-------

Fixed additional comments by Jun Rao


Repository: kafka


Description
-------

KAFKA-1555: provide strong consistency with reasonable availability


Diffs (updated)
-----

  clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java f9de4af 
  clients/src/main/java/org/apache/kafka/common/config/ConfigDef.java addc906 
  clients/src/main/java/org/apache/kafka/common/errors/NotEnoughReplicasAfterAppendException.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/errors/NotEnoughReplicasException.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/protocol/Errors.java d434f42 
  core/src/main/scala/kafka/cluster/Partition.scala ff106b4 
  core/src/main/scala/kafka/common/ErrorMapping.scala 3fae791 
  core/src/main/scala/kafka/common/NotEnoughReplicasAfterAppendException.scala PRE-CREATION 
  core/src/main/scala/kafka/common/NotEnoughReplicasException.scala PRE-CREATION 
  core/src/main/scala/kafka/log/LogConfig.scala 5746ad4 
  core/src/main/scala/kafka/producer/SyncProducerConfig.scala 69b2d0c 
  core/src/main/scala/kafka/server/KafkaApis.scala c584b55 
  core/src/main/scala/kafka/server/KafkaConfig.scala 165c816 
  core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala 39f777b 
  core/src/test/scala/unit/kafka/producer/ProducerTest.scala dd71d81 
  core/src/test/scala/unit/kafka/producer/SyncProducerTest.scala 24deea0 
  core/src/test/scala/unit/kafka/utils/TestUtils.scala 2dbdd3c 

Diff: https://reviews.apache.org/r/25886/diff/


Testing
-------

With 3 broker cluster, created 3 topics each with 1 partition and 3 replicas, with 1,3 and 4 min.insync.replicas.
* min.insync.replicas=1 behaved normally (all writes succeeded as long as a broker was up)
* min.insync.replicas=3 returned NotEnoughReplicas when required.acks=-1 and one broker was down
* min.insync.replicas=4 returned NotEnoughReplicas when required.acks=-1

See notes about retry behavior in the JIRA.


Thanks,

Gwen Shapira

Re: Review Request 25886: KAFKA-1555: provide strong consistency with reasonable availability

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


> On Oct. 2, 2014, 4:08 a.m., Jun Rao wrote:
> > core/src/main/scala/kafka/cluster/Partition.scala, lines 372-373
> > <https://reviews.apache.org/r/25886/diff/7/?file=709987#file709987line372>
> >
> >     Hmm, this code looks a bit weird. It's weird in that a topic level config only takes effect under certain producer side config. My feeling is that if we make min.isr a topic level config, then it should work in the same way independent of the producer side config. So, perhaps it's better to just check inSyncSize < minIsr, irrespective of the ack. What do you think?

Agree its weird. However, that was pretty much what we agreed on in the Jira discussion - if a producer specifies acks=0 and acks=1, they don't care about reliability that much, they certainly don't care about size of ISR (since they don't expect any acks from ISR). I'm pretty sure we decided that min.isr only makes sense with acks=-1, so thats what I implemented.

Since it does look out of place here, and I'm a bit worried about non-producer calls to this function, I'll try to move this check higher up the call stack.

Does that make sense?


- Gwen


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25886/#review55187
-----------------------------------------------------------


On Oct. 1, 2014, 1:19 a.m., Gwen Shapira wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25886/
> -----------------------------------------------------------
> 
> (Updated Oct. 1, 2014, 1:19 a.m.)
> 
> 
> Review request for kafka.
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> KAFKA-1555: provide strong consistency with reasonable availability
> 
> 
> Diffs
> -----
> 
>   clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java f9de4af 
>   clients/src/main/java/org/apache/kafka/common/config/ConfigDef.java addc906 
>   clients/src/main/java/org/apache/kafka/common/errors/NotEnoughReplicasAfterAppendException.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/errors/NotEnoughReplicasException.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/protocol/Errors.java d434f42 
>   core/src/main/scala/kafka/cluster/Partition.scala ff106b4 
>   core/src/main/scala/kafka/common/ErrorMapping.scala 3fae791 
>   core/src/main/scala/kafka/common/NotEnoughReplicasAfterAppendException.scala PRE-CREATION 
>   core/src/main/scala/kafka/common/NotEnoughReplicasException.scala PRE-CREATION 
>   core/src/main/scala/kafka/log/LogConfig.scala 5746ad4 
>   core/src/main/scala/kafka/producer/SyncProducerConfig.scala 69b2d0c 
>   core/src/main/scala/kafka/server/KafkaApis.scala c584b55 
>   core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala 39f777b 
>   core/src/test/scala/unit/kafka/producer/ProducerTest.scala dd71d81 
>   core/src/test/scala/unit/kafka/producer/SyncProducerTest.scala 24deea0 
>   core/src/test/scala/unit/kafka/utils/TestUtils.scala 2dbdd3c 
> 
> Diff: https://reviews.apache.org/r/25886/diff/
> 
> 
> Testing
> -------
> 
> With 3 broker cluster, created 3 topics each with 1 partition and 3 replicas, with 1,3 and 4 min.insync.replicas.
> * min.insync.replicas=1 behaved normally (all writes succeeded as long as a broker was up)
> * min.insync.replicas=3 returned NotEnoughReplicas when required.acks=-1 and one broker was down
> * min.insync.replicas=4 returned NotEnoughReplicas when required.acks=-1
> 
> See notes about retry behavior in the JIRA.
> 
> 
> Thanks,
> 
> Gwen Shapira
> 
>

Re: Review Request 25886: KAFKA-1555: provide strong consistency with reasonable availability

Posted by Jun Rao <ju...@gmail.com>.


> On Oct. 2, 2014, 4:08 a.m., Jun Rao wrote:
> > core/src/main/scala/kafka/cluster/Partition.scala, lines 372-373
> > <https://reviews.apache.org/r/25886/diff/7/?file=709987#file709987line372>
> >
> >     Hmm, this code looks a bit weird. It's weird in that a topic level config only takes effect under certain producer side config. My feeling is that if we make min.isr a topic level config, then it should work in the same way independent of the producer side config. So, perhaps it's better to just check inSyncSize < minIsr, irrespective of the ack. What do you think?
> 
> Gwen Shapira wrote:
>     Agree its weird. However, that was pretty much what we agreed on in the Jira discussion - if a producer specifies acks=0 and acks=1, they don't care about reliability that much, they certainly don't care about size of ISR (since they don't expect any acks from ISR). I'm pretty sure we decided that min.isr only makes sense with acks=-1, so thats what I implemented.
>     
>     Since it does look out of place here, and I'm a bit worried about non-producer calls to this function, I'll try to move this check higher up the call stack.
>     
>     Does that make sense?
> 
> Gwen Shapira wrote:
>     Actually, we can't move it up the call stack since we don't have access to the min.isr value farther up (conf only exists on the leader replica for a partition). I can't think of a better way to fulfill the requirement of min.isr only applies when acks=-1, which IMO is an important requirement.

Ok. Let's leave this as it is then. Could you address the rest of the comments?


- Jun


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25886/#review55187
-----------------------------------------------------------


On Oct. 1, 2014, 1:19 a.m., Gwen Shapira wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25886/
> -----------------------------------------------------------
> 
> (Updated Oct. 1, 2014, 1:19 a.m.)
> 
> 
> Review request for kafka.
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> KAFKA-1555: provide strong consistency with reasonable availability
> 
> 
> Diffs
> -----
> 
>   clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java f9de4af 
>   clients/src/main/java/org/apache/kafka/common/config/ConfigDef.java addc906 
>   clients/src/main/java/org/apache/kafka/common/errors/NotEnoughReplicasAfterAppendException.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/errors/NotEnoughReplicasException.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/protocol/Errors.java d434f42 
>   core/src/main/scala/kafka/cluster/Partition.scala ff106b4 
>   core/src/main/scala/kafka/common/ErrorMapping.scala 3fae791 
>   core/src/main/scala/kafka/common/NotEnoughReplicasAfterAppendException.scala PRE-CREATION 
>   core/src/main/scala/kafka/common/NotEnoughReplicasException.scala PRE-CREATION 
>   core/src/main/scala/kafka/log/LogConfig.scala 5746ad4 
>   core/src/main/scala/kafka/producer/SyncProducerConfig.scala 69b2d0c 
>   core/src/main/scala/kafka/server/KafkaApis.scala c584b55 
>   core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala 39f777b 
>   core/src/test/scala/unit/kafka/producer/ProducerTest.scala dd71d81 
>   core/src/test/scala/unit/kafka/producer/SyncProducerTest.scala 24deea0 
>   core/src/test/scala/unit/kafka/utils/TestUtils.scala 2dbdd3c 
> 
> Diff: https://reviews.apache.org/r/25886/diff/
> 
> 
> Testing
> -------
> 
> With 3 broker cluster, created 3 topics each with 1 partition and 3 replicas, with 1,3 and 4 min.insync.replicas.
> * min.insync.replicas=1 behaved normally (all writes succeeded as long as a broker was up)
> * min.insync.replicas=3 returned NotEnoughReplicas when required.acks=-1 and one broker was down
> * min.insync.replicas=4 returned NotEnoughReplicas when required.acks=-1
> 
> See notes about retry behavior in the JIRA.
> 
> 
> Thanks,
> 
> Gwen Shapira
> 
>

Re: Review Request 25886: KAFKA-1555: provide strong consistency with reasonable availability

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


> On Oct. 2, 2014, 4:08 a.m., Jun Rao wrote:
> > core/src/main/scala/kafka/cluster/Partition.scala, lines 372-373
> > <https://reviews.apache.org/r/25886/diff/7/?file=709987#file709987line372>
> >
> >     Hmm, this code looks a bit weird. It's weird in that a topic level config only takes effect under certain producer side config. My feeling is that if we make min.isr a topic level config, then it should work in the same way independent of the producer side config. So, perhaps it's better to just check inSyncSize < minIsr, irrespective of the ack. What do you think?
> 
> Gwen Shapira wrote:
>     Agree its weird. However, that was pretty much what we agreed on in the Jira discussion - if a producer specifies acks=0 and acks=1, they don't care about reliability that much, they certainly don't care about size of ISR (since they don't expect any acks from ISR). I'm pretty sure we decided that min.isr only makes sense with acks=-1, so thats what I implemented.
>     
>     Since it does look out of place here, and I'm a bit worried about non-producer calls to this function, I'll try to move this check higher up the call stack.
>     
>     Does that make sense?

Actually, we can't move it up the call stack since we don't have access to the min.isr value farther up (conf only exists on the leader replica for a partition). I can't think of a better way to fulfill the requirement of min.isr only applies when acks=-1, which IMO is an important requirement.


- Gwen


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25886/#review55187
-----------------------------------------------------------


On Oct. 1, 2014, 1:19 a.m., Gwen Shapira wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25886/
> -----------------------------------------------------------
> 
> (Updated Oct. 1, 2014, 1:19 a.m.)
> 
> 
> Review request for kafka.
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> KAFKA-1555: provide strong consistency with reasonable availability
> 
> 
> Diffs
> -----
> 
>   clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java f9de4af 
>   clients/src/main/java/org/apache/kafka/common/config/ConfigDef.java addc906 
>   clients/src/main/java/org/apache/kafka/common/errors/NotEnoughReplicasAfterAppendException.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/errors/NotEnoughReplicasException.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/protocol/Errors.java d434f42 
>   core/src/main/scala/kafka/cluster/Partition.scala ff106b4 
>   core/src/main/scala/kafka/common/ErrorMapping.scala 3fae791 
>   core/src/main/scala/kafka/common/NotEnoughReplicasAfterAppendException.scala PRE-CREATION 
>   core/src/main/scala/kafka/common/NotEnoughReplicasException.scala PRE-CREATION 
>   core/src/main/scala/kafka/log/LogConfig.scala 5746ad4 
>   core/src/main/scala/kafka/producer/SyncProducerConfig.scala 69b2d0c 
>   core/src/main/scala/kafka/server/KafkaApis.scala c584b55 
>   core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala 39f777b 
>   core/src/test/scala/unit/kafka/producer/ProducerTest.scala dd71d81 
>   core/src/test/scala/unit/kafka/producer/SyncProducerTest.scala 24deea0 
>   core/src/test/scala/unit/kafka/utils/TestUtils.scala 2dbdd3c 
> 
> Diff: https://reviews.apache.org/r/25886/diff/
> 
> 
> Testing
> -------
> 
> With 3 broker cluster, created 3 topics each with 1 partition and 3 replicas, with 1,3 and 4 min.insync.replicas.
> * min.insync.replicas=1 behaved normally (all writes succeeded as long as a broker was up)
> * min.insync.replicas=3 returned NotEnoughReplicas when required.acks=-1 and one broker was down
> * min.insync.replicas=4 returned NotEnoughReplicas when required.acks=-1
> 
> See notes about retry behavior in the JIRA.
> 
> 
> Thanks,
> 
> Gwen Shapira
> 
>

Re: Review Request 25886: KAFKA-1555: provide strong consistency with reasonable availability

Posted by Jun Rao <ju...@gmail.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25886/#review55187
-----------------------------------------------------------


Thanks for the patch. A few comments below.


clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java
<https://reviews.apache.org/r/25886/#comment95584>

    "all" is more meaningful than -1. So, we can keep it that way. Under the cover, we turn "all" to -1 in the producer request.



core/src/main/scala/kafka/cluster/Partition.scala
<https://reviews.apache.org/r/25886/#comment95588>

    Hmm, this code looks a bit weird. It's weird in that a topic level config only takes effect under certain producer side config. My feeling is that if we make min.isr a topic level config, then it should work in the same way independent of the producer side config. So, perhaps it's better to just check inSyncSize < minIsr, irrespective of the ack. What do you think?



core/src/main/scala/kafka/log/LogConfig.scala
<https://reviews.apache.org/r/25886/#comment95587>

    To be consistent with other per topic configs, we need to define it in KafkaConfig as a global setting.


- Jun Rao


On Oct. 1, 2014, 1:19 a.m., Gwen Shapira wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25886/
> -----------------------------------------------------------
> 
> (Updated Oct. 1, 2014, 1:19 a.m.)
> 
> 
> Review request for kafka.
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> KAFKA-1555: provide strong consistency with reasonable availability
> 
> 
> Diffs
> -----
> 
>   clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java f9de4af 
>   clients/src/main/java/org/apache/kafka/common/config/ConfigDef.java addc906 
>   clients/src/main/java/org/apache/kafka/common/errors/NotEnoughReplicasAfterAppendException.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/errors/NotEnoughReplicasException.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/protocol/Errors.java d434f42 
>   core/src/main/scala/kafka/cluster/Partition.scala ff106b4 
>   core/src/main/scala/kafka/common/ErrorMapping.scala 3fae791 
>   core/src/main/scala/kafka/common/NotEnoughReplicasAfterAppendException.scala PRE-CREATION 
>   core/src/main/scala/kafka/common/NotEnoughReplicasException.scala PRE-CREATION 
>   core/src/main/scala/kafka/log/LogConfig.scala 5746ad4 
>   core/src/main/scala/kafka/producer/SyncProducerConfig.scala 69b2d0c 
>   core/src/main/scala/kafka/server/KafkaApis.scala c584b55 
>   core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala 39f777b 
>   core/src/test/scala/unit/kafka/producer/ProducerTest.scala dd71d81 
>   core/src/test/scala/unit/kafka/producer/SyncProducerTest.scala 24deea0 
>   core/src/test/scala/unit/kafka/utils/TestUtils.scala 2dbdd3c 
> 
> Diff: https://reviews.apache.org/r/25886/diff/
> 
> 
> Testing
> -------
> 
> With 3 broker cluster, created 3 topics each with 1 partition and 3 replicas, with 1,3 and 4 min.insync.replicas.
> * min.insync.replicas=1 behaved normally (all writes succeeded as long as a broker was up)
> * min.insync.replicas=3 returned NotEnoughReplicas when required.acks=-1 and one broker was down
> * min.insync.replicas=4 returned NotEnoughReplicas when required.acks=-1
> 
> See notes about retry behavior in the JIRA.
> 
> 
> Thanks,
> 
> Gwen Shapira
> 
>

Re: Review Request 25886: KAFKA-1555: provide strong consistency with reasonable availability

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

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25886/
-----------------------------------------------------------

(Updated Oct. 1, 2014, 1:19 a.m.)


Review request for kafka.


Changes
-------

Addressed Neha's and June's comments.

Main changes:
* appendMessagesToLeader also checks minISR and acks to avoid writing to log when there are not enough replicas. This means that appendMessagesToLeader now takes an extra argument with acks. I defaulted to acks=0 to retain previous behavior in cases when this is not actually a producer request (Part of the compaction code also appends messages).
* We now validate that acks are in (-1,0,1). For the new producer I added an extra validator because acks was a string and we can't change that without breaking clients. The string validator will be useful when we switch to enum. 
* However, it looks like the new producer does not use the validator, except on the default value. This is a general problem, so I didn't fix it here, but the new producer still accepts acks>1
* If we catch minISR issue before appending message, we throw a NotEnoughReplica exception and there are no duplicates. If we catch minISR issue after appending to log (while waiting for acks), we throw NotEnoughReplicaAfterAppend exception, so the client will be aware of possible duplicates. The new exception should be rare, and I could not figure out a way to test it (unit or other), so its also untested.


Repository: kafka


Description
-------

KAFKA-1555: provide strong consistency with reasonable availability


Diffs (updated)
-----

  clients/src/main/java/org/apache/kafka/clients/producer/ProducerConfig.java f9de4af 
  clients/src/main/java/org/apache/kafka/common/config/ConfigDef.java addc906 
  clients/src/main/java/org/apache/kafka/common/errors/NotEnoughReplicasAfterAppendException.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/errors/NotEnoughReplicasException.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/protocol/Errors.java d434f42 
  core/src/main/scala/kafka/cluster/Partition.scala ff106b4 
  core/src/main/scala/kafka/common/ErrorMapping.scala 3fae791 
  core/src/main/scala/kafka/common/NotEnoughReplicasAfterAppendException.scala PRE-CREATION 
  core/src/main/scala/kafka/common/NotEnoughReplicasException.scala PRE-CREATION 
  core/src/main/scala/kafka/log/LogConfig.scala 5746ad4 
  core/src/main/scala/kafka/producer/SyncProducerConfig.scala 69b2d0c 
  core/src/main/scala/kafka/server/KafkaApis.scala c584b55 
  core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala 39f777b 
  core/src/test/scala/unit/kafka/producer/ProducerTest.scala dd71d81 
  core/src/test/scala/unit/kafka/producer/SyncProducerTest.scala 24deea0 
  core/src/test/scala/unit/kafka/utils/TestUtils.scala 2dbdd3c 

Diff: https://reviews.apache.org/r/25886/diff/


Testing
-------

With 3 broker cluster, created 3 topics each with 1 partition and 3 replicas, with 1,3 and 4 min.insync.replicas.
* min.insync.replicas=1 behaved normally (all writes succeeded as long as a broker was up)
* min.insync.replicas=3 returned NotEnoughReplicas when required.acks=-1 and one broker was down
* min.insync.replicas=4 returned NotEnoughReplicas when required.acks=-1

See notes about retry behavior in the JIRA.


Thanks,

Gwen Shapira

Re: Review Request 25886: KAFKA-1555: provide strong consistency with reasonable availability

Posted by Neha Narkhede <ne...@gmail.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25886/#review54639
-----------------------------------------------------------


I suggest we also make the changes required to reject the write if min.isr > size of current isr.


clients/src/main/java/org/apache/kafka/common/errors/NotEnoughReplicasException.java
<https://reviews.apache.org/r/25886/#comment94876>

    Could you please add a description to this exception class that explains the purpose of the exception?



core/src/main/scala/kafka/cluster/Partition.scala
<https://reviews.apache.org/r/25886/#comment94878>

    can you remove the extra whitespace in requiredOffset )?



core/src/main/scala/kafka/common/NotEnoughReplicasException.scala
<https://reviews.apache.org/r/25886/#comment94879>

    Same here. Could you please add a description?


- Neha Narkhede


On Sept. 25, 2014, 7:41 p.m., Gwen Shapira wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25886/
> -----------------------------------------------------------
> 
> (Updated Sept. 25, 2014, 7:41 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> KAFKA-1555: provide strong consistency with reasonable availability
> 
> 
> Diffs
> -----
> 
>   clients/src/main/java/org/apache/kafka/common/errors/NotEnoughReplicasException.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/protocol/Errors.java d434f42 
>   core/src/main/scala/kafka/cluster/Partition.scala ff106b4 
>   core/src/main/scala/kafka/common/ErrorMapping.scala 3fae791 
>   core/src/main/scala/kafka/common/NotEnoughReplicasException.scala PRE-CREATION 
>   core/src/main/scala/kafka/log/LogConfig.scala 5746ad4 
>   core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala 39f777b 
>   core/src/test/scala/unit/kafka/producer/SyncProducerTest.scala 24deea0 
>   core/src/test/scala/unit/kafka/utils/TestUtils.scala 2dbdd3c 
> 
> Diff: https://reviews.apache.org/r/25886/diff/
> 
> 
> Testing
> -------
> 
> With 3 broker cluster, created 3 topics each with 1 partition and 3 replicas, with 1,3 and 4 min.insync.replicas.
> * min.insync.replicas=1 behaved normally (all writes succeeded as long as a broker was up)
> * min.insync.replicas=3 returned NotEnoughReplicas when required.acks=-1 and one broker was down
> * min.insync.replicas=4 returned NotEnoughReplicas when required.acks=-1
> 
> See notes about retry behavior in the JIRA.
> 
> 
> Thanks,
> 
> Gwen Shapira
> 
>

Re: Review Request 25886: KAFKA-1555: provide strong consistency with reasonable availability

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


> On Sept. 30, 2014, 4:55 p.m., Gwen Shapira wrote:
> > I didn't get a chance to go over the entire review in detail. I did notice that we now have separate objects for MFromConfig and MToConfig. What's the reasoning behind that?

Ignore. commenting on wrong RB.


- Gwen


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25886/#review54987
-----------------------------------------------------------


On Sept. 25, 2014, 7:41 p.m., Gwen Shapira wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25886/
> -----------------------------------------------------------
> 
> (Updated Sept. 25, 2014, 7:41 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> KAFKA-1555: provide strong consistency with reasonable availability
> 
> 
> Diffs
> -----
> 
>   clients/src/main/java/org/apache/kafka/common/errors/NotEnoughReplicasException.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/protocol/Errors.java d434f42 
>   core/src/main/scala/kafka/cluster/Partition.scala ff106b4 
>   core/src/main/scala/kafka/common/ErrorMapping.scala 3fae791 
>   core/src/main/scala/kafka/common/NotEnoughReplicasException.scala PRE-CREATION 
>   core/src/main/scala/kafka/log/LogConfig.scala 5746ad4 
>   core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala 39f777b 
>   core/src/test/scala/unit/kafka/producer/SyncProducerTest.scala 24deea0 
>   core/src/test/scala/unit/kafka/utils/TestUtils.scala 2dbdd3c 
> 
> Diff: https://reviews.apache.org/r/25886/diff/
> 
> 
> Testing
> -------
> 
> With 3 broker cluster, created 3 topics each with 1 partition and 3 replicas, with 1,3 and 4 min.insync.replicas.
> * min.insync.replicas=1 behaved normally (all writes succeeded as long as a broker was up)
> * min.insync.replicas=3 returned NotEnoughReplicas when required.acks=-1 and one broker was down
> * min.insync.replicas=4 returned NotEnoughReplicas when required.acks=-1
> 
> See notes about retry behavior in the JIRA.
> 
> 
> Thanks,
> 
> Gwen Shapira
> 
>

Re: Review Request 25886: KAFKA-1555: provide strong consistency with reasonable availability

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

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25886/#review54987
-----------------------------------------------------------


I didn't get a chance to go over the entire review in detail. I did notice that we now have separate objects for MFromConfig and MToConfig. What's the reasoning behind that?

- Gwen Shapira


On Sept. 25, 2014, 7:41 p.m., Gwen Shapira wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25886/
> -----------------------------------------------------------
> 
> (Updated Sept. 25, 2014, 7:41 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> KAFKA-1555: provide strong consistency with reasonable availability
> 
> 
> Diffs
> -----
> 
>   clients/src/main/java/org/apache/kafka/common/errors/NotEnoughReplicasException.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/protocol/Errors.java d434f42 
>   core/src/main/scala/kafka/cluster/Partition.scala ff106b4 
>   core/src/main/scala/kafka/common/ErrorMapping.scala 3fae791 
>   core/src/main/scala/kafka/common/NotEnoughReplicasException.scala PRE-CREATION 
>   core/src/main/scala/kafka/log/LogConfig.scala 5746ad4 
>   core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala 39f777b 
>   core/src/test/scala/unit/kafka/producer/SyncProducerTest.scala 24deea0 
>   core/src/test/scala/unit/kafka/utils/TestUtils.scala 2dbdd3c 
> 
> Diff: https://reviews.apache.org/r/25886/diff/
> 
> 
> Testing
> -------
> 
> With 3 broker cluster, created 3 topics each with 1 partition and 3 replicas, with 1,3 and 4 min.insync.replicas.
> * min.insync.replicas=1 behaved normally (all writes succeeded as long as a broker was up)
> * min.insync.replicas=3 returned NotEnoughReplicas when required.acks=-1 and one broker was down
> * min.insync.replicas=4 returned NotEnoughReplicas when required.acks=-1
> 
> See notes about retry behavior in the JIRA.
> 
> 
> Thanks,
> 
> Gwen Shapira
> 
>

Re: Review Request 25886: KAFKA-1555: provide strong consistency with reasonable availability

Posted by Jun Rao <ju...@gmail.com>.


> On Sept. 26, 2014, 12:17 a.m., Jun Rao wrote:
> > Thanks for the patch. Looks good overall. A couple of comments.
> > 
> > 1. Let's add a check of min.isr in Partition.appendMessagesToLeader() so that the message is not added to the leader's log if isr is less than min.isr.
> > 2. We talked about dropping the ack > 1 support in the jira. However, I think that's easier done after kafka-1583 is completed. So, we don't have to do this change here. I will file a follow up jira.
> 
> Jun Rao wrote:
>     1. We can give a different exception in this case, sth like RejectMessageDueToNotEnoughReplicas.
> 
> Gwen Shapira wrote:
>     Jun,
>     Regarding #1 (check for min.isr in appendMessageToLeader):
>     
>     From what I see, appendMessageToLeader happens before we do checkEnoughReplicasReachOffset. 
>     If appendMessageToLeader throws a NotEnoughReplicas exception, the message will never make it to purgatory and checkEnoughReplicas will never get called at all.
>     
>     However, I don't see anything preventing the ISR from shrinking after we wrote to the leader. In which case checkEnoughReplicas will throw a NotEnoughReplicas exception for a message that was already written to the leader.
>     
>     Maybe we need to different exceptions so producers can tell the difference between the cases?

Gwen,

Yes, it would be good to have two different exceptions for these cases. Also, let's remove the ack > 2 support from the doc in ProducerConfig (both the old and the new producer) and add a check to make sure that the provided value is valid.


- Jun


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25886/#review54635
-----------------------------------------------------------


On Sept. 25, 2014, 7:41 p.m., Gwen Shapira wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25886/
> -----------------------------------------------------------
> 
> (Updated Sept. 25, 2014, 7:41 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> KAFKA-1555: provide strong consistency with reasonable availability
> 
> 
> Diffs
> -----
> 
>   clients/src/main/java/org/apache/kafka/common/errors/NotEnoughReplicasException.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/protocol/Errors.java d434f42 
>   core/src/main/scala/kafka/cluster/Partition.scala ff106b4 
>   core/src/main/scala/kafka/common/ErrorMapping.scala 3fae791 
>   core/src/main/scala/kafka/common/NotEnoughReplicasException.scala PRE-CREATION 
>   core/src/main/scala/kafka/log/LogConfig.scala 5746ad4 
>   core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala 39f777b 
>   core/src/test/scala/unit/kafka/producer/SyncProducerTest.scala 24deea0 
>   core/src/test/scala/unit/kafka/utils/TestUtils.scala 2dbdd3c 
> 
> Diff: https://reviews.apache.org/r/25886/diff/
> 
> 
> Testing
> -------
> 
> With 3 broker cluster, created 3 topics each with 1 partition and 3 replicas, with 1,3 and 4 min.insync.replicas.
> * min.insync.replicas=1 behaved normally (all writes succeeded as long as a broker was up)
> * min.insync.replicas=3 returned NotEnoughReplicas when required.acks=-1 and one broker was down
> * min.insync.replicas=4 returned NotEnoughReplicas when required.acks=-1
> 
> See notes about retry behavior in the JIRA.
> 
> 
> Thanks,
> 
> Gwen Shapira
> 
>

Re: Review Request 25886: KAFKA-1555: provide strong consistency with reasonable availability

Posted by Jun Rao <ju...@gmail.com>.


> On Sept. 26, 2014, 12:17 a.m., Jun Rao wrote:
> > Thanks for the patch. Looks good overall. A couple of comments.
> > 
> > 1. Let's add a check of min.isr in Partition.appendMessagesToLeader() so that the message is not added to the leader's log if isr is less than min.isr.
> > 2. We talked about dropping the ack > 1 support in the jira. However, I think that's easier done after kafka-1583 is completed. So, we don't have to do this change here. I will file a follow up jira.

1. We can give a different exception in this case, sth like RejectMessageDueToNotEnoughReplicas.


- Jun


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25886/#review54635
-----------------------------------------------------------


On Sept. 25, 2014, 7:41 p.m., Gwen Shapira wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25886/
> -----------------------------------------------------------
> 
> (Updated Sept. 25, 2014, 7:41 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> KAFKA-1555: provide strong consistency with reasonable availability
> 
> 
> Diffs
> -----
> 
>   clients/src/main/java/org/apache/kafka/common/errors/NotEnoughReplicasException.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/protocol/Errors.java d434f42 
>   core/src/main/scala/kafka/cluster/Partition.scala ff106b4 
>   core/src/main/scala/kafka/common/ErrorMapping.scala 3fae791 
>   core/src/main/scala/kafka/common/NotEnoughReplicasException.scala PRE-CREATION 
>   core/src/main/scala/kafka/log/LogConfig.scala 5746ad4 
>   core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala 39f777b 
>   core/src/test/scala/unit/kafka/producer/SyncProducerTest.scala 24deea0 
>   core/src/test/scala/unit/kafka/utils/TestUtils.scala 2dbdd3c 
> 
> Diff: https://reviews.apache.org/r/25886/diff/
> 
> 
> Testing
> -------
> 
> With 3 broker cluster, created 3 topics each with 1 partition and 3 replicas, with 1,3 and 4 min.insync.replicas.
> * min.insync.replicas=1 behaved normally (all writes succeeded as long as a broker was up)
> * min.insync.replicas=3 returned NotEnoughReplicas when required.acks=-1 and one broker was down
> * min.insync.replicas=4 returned NotEnoughReplicas when required.acks=-1
> 
> See notes about retry behavior in the JIRA.
> 
> 
> Thanks,
> 
> Gwen Shapira
> 
>

Re: Review Request 25886: KAFKA-1555: provide strong consistency with reasonable availability

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


> On Sept. 26, 2014, 12:17 a.m., Jun Rao wrote:
> > Thanks for the patch. Looks good overall. A couple of comments.
> > 
> > 1. Let's add a check of min.isr in Partition.appendMessagesToLeader() so that the message is not added to the leader's log if isr is less than min.isr.
> > 2. We talked about dropping the ack > 1 support in the jira. However, I think that's easier done after kafka-1583 is completed. So, we don't have to do this change here. I will file a follow up jira.
> 
> Jun Rao wrote:
>     1. We can give a different exception in this case, sth like RejectMessageDueToNotEnoughReplicas.

Jun,
Regarding #1 (check for min.isr in appendMessageToLeader):

>From what I see, appendMessageToLeader happens before we do checkEnoughReplicasReachOffset. 
If appendMessageToLeader throws a NotEnoughReplicas exception, the message will never make it to purgatory and checkEnoughReplicas will never get called at all.

However, I don't see anything preventing the ISR from shrinking after we wrote to the leader. In which case checkEnoughReplicas will throw a NotEnoughReplicas exception for a message that was already written to the leader.

Maybe we need to different exceptions so producers can tell the difference between the cases?


- Gwen


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25886/#review54635
-----------------------------------------------------------


On Sept. 25, 2014, 7:41 p.m., Gwen Shapira wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25886/
> -----------------------------------------------------------
> 
> (Updated Sept. 25, 2014, 7:41 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> KAFKA-1555: provide strong consistency with reasonable availability
> 
> 
> Diffs
> -----
> 
>   clients/src/main/java/org/apache/kafka/common/errors/NotEnoughReplicasException.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/protocol/Errors.java d434f42 
>   core/src/main/scala/kafka/cluster/Partition.scala ff106b4 
>   core/src/main/scala/kafka/common/ErrorMapping.scala 3fae791 
>   core/src/main/scala/kafka/common/NotEnoughReplicasException.scala PRE-CREATION 
>   core/src/main/scala/kafka/log/LogConfig.scala 5746ad4 
>   core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala 39f777b 
>   core/src/test/scala/unit/kafka/producer/SyncProducerTest.scala 24deea0 
>   core/src/test/scala/unit/kafka/utils/TestUtils.scala 2dbdd3c 
> 
> Diff: https://reviews.apache.org/r/25886/diff/
> 
> 
> Testing
> -------
> 
> With 3 broker cluster, created 3 topics each with 1 partition and 3 replicas, with 1,3 and 4 min.insync.replicas.
> * min.insync.replicas=1 behaved normally (all writes succeeded as long as a broker was up)
> * min.insync.replicas=3 returned NotEnoughReplicas when required.acks=-1 and one broker was down
> * min.insync.replicas=4 returned NotEnoughReplicas when required.acks=-1
> 
> See notes about retry behavior in the JIRA.
> 
> 
> Thanks,
> 
> Gwen Shapira
> 
>

Re: Review Request 25886: KAFKA-1555: provide strong consistency with reasonable availability

Posted by Jun Rao <ju...@gmail.com>.

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25886/#review54635
-----------------------------------------------------------


Thanks for the patch. Looks good overall. A couple of comments.

1. Let's add a check of min.isr in Partition.appendMessagesToLeader() so that the message is not added to the leader's log if isr is less than min.isr.
2. We talked about dropping the ack > 1 support in the jira. However, I think that's easier done after kafka-1583 is completed. So, we don't have to do this change here. I will file a follow up jira.

- Jun Rao


On Sept. 25, 2014, 7:41 p.m., Gwen Shapira wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/25886/
> -----------------------------------------------------------
> 
> (Updated Sept. 25, 2014, 7:41 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Repository: kafka
> 
> 
> Description
> -------
> 
> KAFKA-1555: provide strong consistency with reasonable availability
> 
> 
> Diffs
> -----
> 
>   clients/src/main/java/org/apache/kafka/common/errors/NotEnoughReplicasException.java PRE-CREATION 
>   clients/src/main/java/org/apache/kafka/common/protocol/Errors.java d434f42 
>   core/src/main/scala/kafka/cluster/Partition.scala ff106b4 
>   core/src/main/scala/kafka/common/ErrorMapping.scala 3fae791 
>   core/src/main/scala/kafka/common/NotEnoughReplicasException.scala PRE-CREATION 
>   core/src/main/scala/kafka/log/LogConfig.scala 5746ad4 
>   core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala 39f777b 
>   core/src/test/scala/unit/kafka/producer/SyncProducerTest.scala 24deea0 
>   core/src/test/scala/unit/kafka/utils/TestUtils.scala 2dbdd3c 
> 
> Diff: https://reviews.apache.org/r/25886/diff/
> 
> 
> Testing
> -------
> 
> With 3 broker cluster, created 3 topics each with 1 partition and 3 replicas, with 1,3 and 4 min.insync.replicas.
> * min.insync.replicas=1 behaved normally (all writes succeeded as long as a broker was up)
> * min.insync.replicas=3 returned NotEnoughReplicas when required.acks=-1 and one broker was down
> * min.insync.replicas=4 returned NotEnoughReplicas when required.acks=-1
> 
> See notes about retry behavior in the JIRA.
> 
> 
> Thanks,
> 
> Gwen Shapira
> 
>

Re: Review Request 25886: KAFKA-1555: provide strong consistency with reasonable availability

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

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25886/
-----------------------------------------------------------

(Updated Sept. 25, 2014, 7:41 p.m.)


Review request for kafka.


Changes
-------

Removed accidental build.gradle


Repository: kafka


Description
-------

KAFKA-1555: provide strong consistency with reasonable availability


Diffs (updated)
-----

  clients/src/main/java/org/apache/kafka/common/errors/NotEnoughReplicasException.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/protocol/Errors.java d434f42 
  core/src/main/scala/kafka/cluster/Partition.scala ff106b4 
  core/src/main/scala/kafka/common/ErrorMapping.scala 3fae791 
  core/src/main/scala/kafka/common/NotEnoughReplicasException.scala PRE-CREATION 
  core/src/main/scala/kafka/log/LogConfig.scala 5746ad4 
  core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala 39f777b 
  core/src/test/scala/unit/kafka/producer/SyncProducerTest.scala 24deea0 
  core/src/test/scala/unit/kafka/utils/TestUtils.scala 2dbdd3c 

Diff: https://reviews.apache.org/r/25886/diff/


Testing
-------

With 3 broker cluster, created 3 topics each with 1 partition and 3 replicas, with 1,3 and 4 min.insync.replicas.
* min.insync.replicas=1 behaved normally (all writes succeeded as long as a broker was up)
* min.insync.replicas=3 returned NotEnoughReplicas when required.acks=-1 and one broker was down
* min.insync.replicas=4 returned NotEnoughReplicas when required.acks=-1

See notes about retry behavior in the JIRA.


Thanks,

Gwen Shapira

Re: Review Request 25886: KAFKA-1555: provide strong consistency with reasonable availability

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

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25886/
-----------------------------------------------------------

(Updated Sept. 25, 2014, 5:23 a.m.)


Review request for kafka.


Changes
-------

Fixed few issues pointed out by Jun in the review.


Repository: kafka


Description
-------

KAFKA-1555: provide strong consistency with reasonable availability


Diffs (updated)
-----

  build.gradle f6e4f8b 
  clients/src/main/java/org/apache/kafka/common/errors/NotEnoughReplicasException.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/protocol/Errors.java d434f42 
  core/src/main/scala/kafka/cluster/Partition.scala ff106b4 
  core/src/main/scala/kafka/common/ErrorMapping.scala 3fae791 
  core/src/main/scala/kafka/common/NotEnoughReplicasException.scala PRE-CREATION 
  core/src/main/scala/kafka/log/LogConfig.scala 5746ad4 
  core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala 39f777b 
  core/src/test/scala/unit/kafka/producer/SyncProducerTest.scala 24deea0 
  core/src/test/scala/unit/kafka/utils/TestUtils.scala 2dbdd3c 

Diff: https://reviews.apache.org/r/25886/diff/


Testing
-------

With 3 broker cluster, created 3 topics each with 1 partition and 3 replicas, with 1,3 and 4 min.insync.replicas.
* min.insync.replicas=1 behaved normally (all writes succeeded as long as a broker was up)
* min.insync.replicas=3 returned NotEnoughReplicas when required.acks=-1 and one broker was down
* min.insync.replicas=4 returned NotEnoughReplicas when required.acks=-1

See notes about retry behavior in the JIRA.


Thanks,

Gwen Shapira

Re: Review Request 25886: KAFKA-1555: provide strong consistency with reasonable availability

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

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25886/
-----------------------------------------------------------

(Updated Sept. 23, 2014, 12:28 a.m.)


Review request for kafka.


Repository: kafka


Description
-------

KAFKA-1555: provide strong consistency with reasonable availability


Diffs (updated)
-----

  clients/src/main/java/org/apache/kafka/common/errors/NotEnoughReplicasException.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/protocol/Errors.java d434f42 
  core/src/main/scala/kafka/cluster/Partition.scala ff106b4 
  core/src/main/scala/kafka/common/ErrorMapping.scala 3fae791 
  core/src/main/scala/kafka/common/NotEnoughReplicasException.scala PRE-CREATION 
  core/src/main/scala/kafka/log/LogConfig.scala 5746ad4 
  core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala 39f777b 
  core/src/test/scala/unit/kafka/producer/SyncProducerTest.scala 24deea0 
  core/src/test/scala/unit/kafka/utils/TestUtils.scala 2dbdd3c 

Diff: https://reviews.apache.org/r/25886/diff/


Testing
-------

With 3 broker cluster, created 3 topics each with 1 partition and 3 replicas, with 1,3 and 4 min.insync.replicas.
* min.insync.replicas=1 behaved normally (all writes succeeded as long as a broker was up)
* min.insync.replicas=3 returned NotEnoughReplicas when required.acks=-1 and one broker was down
* min.insync.replicas=4 returned NotEnoughReplicas when required.acks=-1

See notes about retry behavior in the JIRA.


Thanks,

Gwen Shapira

Re: Review Request 25886: KAFKA-1555: provide strong consistency with reasonable availability

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

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25886/
-----------------------------------------------------------

(Updated Sept. 23, 2014, 12:05 a.m.)


Review request for kafka.


Changes
-------

Fixes following comments in JIRA


Repository: kafka


Description
-------

KAFKA-1555: provide strong consistency with reasonable availability


Diffs (updated)
-----

  clients/src/main/java/org/apache/kafka/common/errors/NotEnoughReplicasException.java PRE-CREATION 
  clients/src/main/java/org/apache/kafka/common/protocol/Errors.java d434f42 
  core/src/main/scala/kafka/cluster/Partition.scala ff106b4 
  core/src/main/scala/kafka/common/ErrorMapping.scala 3fae791 
  core/src/main/scala/kafka/common/NotEnoughReplicasException.scala PRE-CREATION 
  core/src/main/scala/kafka/log/LogConfig.scala 5746ad4 
  core/src/main/scala/kafka/server/KafkaApis.scala c584b55 
  core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala 39f777b 
  core/src/test/scala/unit/kafka/producer/SyncProducerTest.scala 24deea0 
  core/src/test/scala/unit/kafka/utils/TestUtils.scala 2dbdd3c 

Diff: https://reviews.apache.org/r/25886/diff/


Testing
-------

With 3 broker cluster, created 3 topics each with 1 partition and 3 replicas, with 1,3 and 4 min.insync.replicas.
* min.insync.replicas=1 behaved normally (all writes succeeded as long as a broker was up)
* min.insync.replicas=3 returned NotEnoughReplicas when required.acks=-1 and one broker was down
* min.insync.replicas=4 returned NotEnoughReplicas when required.acks=-1

See notes about retry behavior in the JIRA.


Thanks,

Gwen Shapira

Re: Review Request 25886: KAFKA-1555: provide strong consistency with reasonable availability

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

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/25886/
-----------------------------------------------------------

(Updated Sept. 22, 2014, 5:59 a.m.)


Review request for kafka.


Repository: kafka


Description
-------

KAFKA-1555: provide strong consistency with reasonable availability


Diffs (updated)
-----

  core/src/main/scala/kafka/cluster/Partition.scala ff106b4 
  core/src/main/scala/kafka/common/ErrorMapping.scala 3fae791 
  core/src/main/scala/kafka/common/NotEnoughReplicasException.scala PRE-CREATION 
  core/src/main/scala/kafka/log/LogConfig.scala 5746ad4 

Diff: https://reviews.apache.org/r/25886/diff/


Testing
-------

With 3 broker cluster, created 3 topics each with 1 partition and 3 replicas, with 1,3 and 4 min.insync.replicas.
* min.insync.replicas=1 behaved normally (all writes succeeded as long as a broker was up)
* min.insync.replicas=3 returned NotEnoughReplicas when required.acks=-1 and one broker was down
* min.insync.replicas=4 returned NotEnoughReplicas when required.acks=-1

See notes about retry behavior in the JIRA.


Thanks,

Gwen Shapira