> On Feb. 10, 2015, 8:12 p.m., Joel Koshy wrote: > > core/src/main/scala/kafka/server/KafkaApis.scala, line 166 > > <https://reviews.apache.org/r/29647/diff/2/?file=822000#file822000line166> > > > > Rather than do this here in kafka-apis, how about moving this to > > ReplicaManager.appendMessages? I think there are a few potential benefits: > > * It keeps kafka-apis simpler as a dispatcher of requests without > > looking into the requests (albeit we currently break that all over the > > place but improved after KAFKA-1583). > > * You can just call the responseCallBack from there instead of > > explicitly creating the response object and sending via the request channel. > > * It may be easier to test your change - i.e., no need the full-fledged > > kafka-apis test. You just need to create a replicaManager with a mock > > zkclient and mock scheduler and test the appendMessages call.
Yeah. I think the code for handling errors early is a bit messy and its a question of where we prefer the mess :) I went with handleProducerRequest() since appendMessages() already had some complex logic in it and I prefered not to complicate things farther. The handler code already had special cases for requiredAcks == 0, so it looked like the right place. Anyway, the point on easier testing convinced me, will move the logic to appendMessages :) - Gwen ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/29647/#review71842 ----------------------------------------------------------- On Jan. 14, 2015, 11:41 p.m., Gwen Shapira wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/29647/ > ----------------------------------------------------------- > > (Updated Jan. 14, 2015, 11:41 p.m.) > > > Review request for kafka. > > > Bugs: KAFKA-1697 > https://issues.apache.org/jira/browse/KAFKA-1697 > > > Repository: kafka > > > Description > ------- > > trivial change to add byte serializer to ProducerPerformance; patched by Jun > Rao > > > KAFKA-1723 (delta patch to fix javadoc); make the metrics name in new > producer more standard; patched by Manikumar Reddy; reviewed by Jun Rao > > > removed broker code for handling acks>1 and made > NotEnoughReplicasAfterAppendException non-retriable > > > added early handling of invalid number of acks to handler and a test > > > 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 > 75c80a97e43089cb3f924a38f86d67b5a8dd2b89 > clients/src/main/java/org/apache/kafka/common/protocol/Errors.java > 3316b6a1098311b8603a4a5893bf57b75d2e43cb > core/src/main/scala/kafka/cluster/Partition.scala > b230e9a1fb1a3161f4c9d164e4890a16eceb2ad4 > core/src/main/scala/kafka/server/KafkaApis.scala > c011a1b79bd6c4e832fe7d097daacb0d647d1cd4 > core/src/test/scala/unit/kafka/api/RequestResponseSerializationTest.scala > cd16ced5465d098be7a60498326b2a98c248f343 > core/src/test/scala/unit/kafka/api/testKafkaApis.scala PRE-CREATION > > Diff: https://reviews.apache.org/r/29647/diff/ > > > Testing > ------- > > > Thanks, > > Gwen Shapira > >
