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

[Gwen Shapira]

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

[Jun Rao]

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


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

[Gwen Shapira]

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

[Jun Rao]

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

[Gwen Shapira]

> On Oct. 8, 2014, 2:04 a.m., Joel Koshy wrote:
> > clients/src/main/java/org/apache/kafka/common/config/ConfigDef.java, line 
> > 271
> > 
> >
> > 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

[Joel Koshy]

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


Where is this used?



core/src/main/scala/kafka/cluster/Partition.scala


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


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

[Gwen Shapira]

> On Oct. 8, 2014, midnight, Jun Rao wrote:
> > core/src/main/scala/kafka/cluster/Partition.scala, line 287
> > 
> >
> > 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

[Gwen Shapira]

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

[Jun Rao]

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


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


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



core/src/main/scala/kafka/cluster/Partition.scala


The error code should be NotEnoughReplicasAfterAppendCode.



core/src/main/scala/kafka/common/NotEnoughReplicasException.scala


Probably add that messages are rejected.



core/src/main/scala/kafka/log/LogConfig.scala


-1 (or all)



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


-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

[Gwen Shapira]

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

[Jun Rao]

> On Oct. 2, 2014, 4:08 a.m., Jun Rao wrote:
> > core/src/main/scala/kafka/cluster/Partition.scala, lines 372-373
> > 
> >
> > 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

[Gwen Shapira]

> On Oct. 2, 2014, 4:08 a.m., Jun Rao wrote:
> > core/src/main/scala/kafka/cluster/Partition.scala, lines 372-373
> > 
> >
> > 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

[Gwen Shapira]

> On Oct. 2, 2014, 4:08 a.m., Jun Rao wrote:
> > core/src/main/scala/kafka/cluster/Partition.scala, lines 372-373
> > 
> >
> > 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

[Jun Rao]

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


"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


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


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

[Gwen Shapira]

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

[Gwen Shapira]

> 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

[Gwen Shapira]

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

[Jun Rao]

> 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

[Gwen Shapira]

> 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

[Jun Rao]

> 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

[Neha Narkhede]

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


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


can you remove the extra whitespace in requiredOffset )?



core/src/main/scala/kafka/common/NotEnoughReplicasException.scala


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

[Jun Rao]

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

[Gwen Shapira]

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

[Gwen Shapira]

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

[Gwen Shapira]

> On Sept. 24, 2014, 5:06 a.m., Jun Rao wrote:
> > core/src/main/scala/kafka/cluster/Partition.scala, lines 272-276
> > 
> >
> > 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

[Jun Rao]

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


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



core/src/main/scala/kafka/cluster/Partition.scala


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



core/src/main/scala/kafka/log/LogConfig.scala


Lower case Topic?



core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala


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



core/src/test/scala/integration/kafka/api/ProducerFailureHandlingTest.scala


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

[Gwen Shapira]

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

[Gwen Shapira]

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

[Gwen Shapira]

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

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

[Gwen Shapira]

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