> On Feb. 11, 2015, 1:48 p.m., Joel Koshy wrote: > > core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala, line 96 > > <https://reviews.apache.org/r/29647/diff/3/?file=860605#file860605line96> > > > > Do we need this here?
IMO, its a good idea to have this in any test that starts new threads - to verify that we close them. We have this in a bunch of places. > On Feb. 11, 2015, 1:48 p.m., Joel Koshy wrote: > > core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala, line 83 > > <https://reviews.apache.org/r/29647/diff/3/?file=860605#file860605line83> > > > > Can we just do the assert here instead of throwing an exception? > > > > i.e., `assertEquals(responseStatus.values.size, > > responseStatus.values.count(_.error == INVALID_REQUIRED_ACKS))` > > > > Either way is fine. Whichever is clearer although the above may be more > > concise if it works. good idea. > On Feb. 11, 2015, 1:48 p.m., Joel Koshy wrote: > > clients/src/main/java/org/apache/kafka/common/errors/NotEnoughReplicasAfterAppendException.java, > > line 23 > > <https://reviews.apache.org/r/29647/diff/3/?file=860600#file860600line23> > > > > Pending discussion. changing back to retriable, per discussion in mailing list. I'm leaving this as a separate exception and error code, in case client developers want to do something with the extra information. - Gwen ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29647/#review71960 ----------------------------------------------------------- On Feb. 11, 2015, 1:06 a.m., Gwen Shapira wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/29647/ > ----------------------------------------------------------- > > (Updated Feb. 11, 2015, 1:06 a.m.) > > > Review request for kafka. > > > Bugs: KAFKA-1697 > https://issues.apache.org/jira/browse/KAFKA-1697 > > > Repository: kafka > > > Description > ------- > > added early handling of invalid number of acks to handler and a test > > > merging with current trunk > > > moved check for legal requiredAcks to append and fixed the tests accordingly > > > Diffs > ----- > > > clients/src/main/java/org/apache/kafka/common/errors/InvalidRequiredAcksException.java > PRE-CREATION > > clients/src/main/java/org/apache/kafka/common/errors/NotEnoughReplicasAfterAppendException.java > a6107b818947d6d6818c85cdffcb2b13f69a55c0 > clients/src/main/java/org/apache/kafka/common/protocol/Errors.java > a8deac4ce5149129d0a6f44c0526af9d55649a36 > core/src/main/scala/kafka/cluster/Partition.scala > e6ad8be5e33b6fb31c078ad78f8de709869ddc04 > core/src/main/scala/kafka/server/KafkaApis.scala > 6ee7d8819a9ef923f3a65c865a0a3d8ded8845f0 > core/src/main/scala/kafka/server/ReplicaManager.scala > fb948b9ab28c516e81dab14dcbe211dcd99842b6 > core/src/test/scala/unit/kafka/server/ReplicaManagerTest.scala > faa907131ed0aa94a7eacb78c1ffb576062be87a > > Diff: https://reviews.apache.org/r/29647/diff/ > > > Testing > ------- > > > Thanks, > > Gwen Shapira > >
