Re: Review Request 36578: Patch for KAFKA-2338

[Edward Ribeiro]

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



core/src/main/scala/kafka/admin/TopicCommand.scala (line 92)


Sure! Going to address this.



core/src/main/scala/kafka/server/AbstractFetcherThread.scala (lines 176 - 178)


Cool! Thanks for the heads up! Gonna get rid of this useless catch and 
modify accordingly.


- Edward Ribeiro


On Sept. 2, 2015, 10:30 p.m., Edward Ribeiro wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36578/
> ---
> 
> (Updated Sept. 2, 2015, 10:30 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-2338
> https://issues.apache.org/jira/browse/KAFKA-2338
> 
> 
> Repository: kafka
> 
> 
> Description
> ---
> 
> KAFKA-2338 Warn users if they change max.message.bytes that they also need to 
> update broker and consumer settings
> 
> 
> Diffs
> -
> 
>   core/src/main/scala/kafka/admin/TopicCommand.scala 
> f1405a5b2961bc826caa22507db8ba39ce1cf4d3 
>   core/src/main/scala/kafka/server/AbstractFetcherThread.scala 
> dca975ca1bf3e560b9d9817e7f2a511ef4296e70 
> 
> Diff: https://reviews.apache.org/r/36578/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Edward Ribeiro
> 
>

Re: Review Request 36578: Patch for KAFKA-2338

[Jun Rao]

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


Thanks for the patch. A couple of comments below.


core/src/main/scala/kafka/admin/TopicCommand.scala (line 92)


Could we log what broker and consumer properties that the user need to 
change? Also, we should say what value the user needs to change to. This is a 
bit tricky with keys. So, the fetch size should be >= (max message size + key 
size + overhead).



core/src/main/scala/kafka/server/AbstractFetcherThread.scala (lines 176 - 178)


As Ewen pointed out earlier, this doesn't quite work. The broker never 
returns ErrorMapping.MessageSizeTooLargeCode. It's the responsibility of the 
consumer to check oversized messages. The way you check that is if 
messageSet.sizeInBytes > 0 && messageSet.validBytes <= 0, then there is an 
oversided message. Also, we should probably do the logging in 
ReplicaFetcherThread.processPartitionData() since for regular consumers, we 
already check oversized messages in ConsumerIterator.makeNext().


- Jun Rao


On Sept. 2, 2015, 10:30 p.m., Edward Ribeiro wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/36578/
> ---
> 
> (Updated Sept. 2, 2015, 10:30 p.m.)
> 
> 
> Review request for kafka.
> 
> 
> Bugs: KAFKA-2338
> https://issues.apache.org/jira/browse/KAFKA-2338
> 
> 
> Repository: kafka
> 
> 
> Description
> ---
> 
> KAFKA-2338 Warn users if they change max.message.bytes that they also need to 
> update broker and consumer settings
> 
> 
> Diffs
> -
> 
>   core/src/main/scala/kafka/admin/TopicCommand.scala 
> f1405a5b2961bc826caa22507db8ba39ce1cf4d3 
>   core/src/main/scala/kafka/server/AbstractFetcherThread.scala 
> dca975ca1bf3e560b9d9817e7f2a511ef4296e70 
> 
> Diff: https://reviews.apache.org/r/36578/diff/
> 
> 
> Testing
> ---
> 
> 
> Thanks,
> 
> Edward Ribeiro
> 
>

Re: Review Request 36578: Patch for KAFKA-2338

[Edward Ribeiro]

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

(Updated Sept. 2, 2015, 10:30 p.m.)


Review request for kafka.


Bugs: KAFKA-2338
https://issues.apache.org/jira/browse/KAFKA-2338


Repository: kafka


Description
---

KAFKA-2338 Warn users if they change max.message.bytes that they also need to 
update broker and consumer settings


Diffs (updated)
-

  core/src/main/scala/kafka/admin/TopicCommand.scala 
f1405a5b2961bc826caa22507db8ba39ce1cf4d3 
  core/src/main/scala/kafka/server/AbstractFetcherThread.scala 
dca975ca1bf3e560b9d9817e7f2a511ef4296e70 

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


Testing
---


Thanks,

Edward Ribeiro

Re: Review Request 36578: Patch for KAFKA-2338

[Edward Ribeiro]

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

(Updated Aug. 24, 2015, 5:36 p.m.)


Review request for kafka.


Bugs: KAFKA-2338
https://issues.apache.org/jira/browse/KAFKA-2338


Repository: kafka


Description
---

KAFKA-2338 Warn users if they change max.message.bytes that they also need to 
update broker and consumer settings


Diffs (updated)
-

  core/src/main/scala/kafka/admin/TopicCommand.scala 
f1405a5b2961bc826caa22507db8ba39ce1cf4d3 
  core/src/main/scala/kafka/server/AbstractFetcherThread.scala 
f84306143c43049e3aa44e42beaffe7eb2783163 

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


Testing
---


Thanks,

Edward Ribeiro

Re: Review Request 36578: Patch for KAFKA-2338

[Manikumar Reddy O]

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



core/src/main/scala/kafka/admin/TopicCommand.scala (line 89)
https://reviews.apache.org/r/36578/#comment151144

The warning message can be : WARNING: %s has been increased beyond the 
default max value of %d, update producer and consumer settings as well


Also do want to include patch for second point given in JIRA decription..

- Manikumar Reddy O


On July 21, 2015, 4:21 p.m., Edward Ribeiro wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/36578/
 ---
 
 (Updated July 21, 2015, 4:21 p.m.)
 
 
 Review request for kafka.
 
 
 Bugs: KAFKA-2338
 https://issues.apache.org/jira/browse/KAFKA-2338
 
 
 Repository: kafka
 
 
 Description
 ---
 
 KAFKA-2338 Warn users if they change max.message.bytes that they also need to 
 update broker and consumer settings
 
 
 Diffs
 -
 
   core/src/main/scala/kafka/admin/TopicCommand.scala 
 4e28bf1c08414e8e96e6ca639b927d51bfeb4616 
 
 Diff: https://reviews.apache.org/r/36578/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Edward Ribeiro
 

Re: Review Request 36578: Patch for KAFKA-2338

[Manikumar Reddy O]

On Aug. 20, 2015, 11:07 a.m., Edward Ribeiro wrote:
  Also do want to include patch for second point given in JIRA decription..

just read previous reviews..ignore my comment.


- Manikumar Reddy


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


On July 21, 2015, 4:21 p.m., Edward Ribeiro wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/36578/
 ---
 
 (Updated July 21, 2015, 4:21 p.m.)
 
 
 Review request for kafka.
 
 
 Bugs: KAFKA-2338
 https://issues.apache.org/jira/browse/KAFKA-2338
 
 
 Repository: kafka
 
 
 Description
 ---
 
 KAFKA-2338 Warn users if they change max.message.bytes that they also need to 
 update broker and consumer settings
 
 
 Diffs
 -
 
   core/src/main/scala/kafka/admin/TopicCommand.scala 
 4e28bf1c08414e8e96e6ca639b927d51bfeb4616 
 
 Diff: https://reviews.apache.org/r/36578/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Edward Ribeiro
 

Re: Review Request 36578: Patch for KAFKA-2338

[Edward Ribeiro]

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

(Updated July 21, 2015, 4:21 p.m.)


Review request for kafka.


Bugs: KAFKA-2338
https://issues.apache.org/jira/browse/KAFKA-2338


Repository: kafka


Description
---

KAFKA-2338 Warn users if they change max.message.bytes that they also need to 
update broker and consumer settings


Diffs (updated)
-

  core/src/main/scala/kafka/admin/TopicCommand.scala 
4e28bf1c08414e8e96e6ca639b927d51bfeb4616 

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


Testing
---


Thanks,

Edward Ribeiro

Re: Review Request 36578: Patch for KAFKA-2338

[Ewen Cheslack-Postava]

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



core/src/main/scala/kafka/server/AbstractFetcherThread.scala (line 166)
https://reviews.apache.org/r/36578/#comment146731

Well, 2 things. First, I only took a quick look after I couldn't reproduce 
the issue, so I could have missed an error code that can be returned and which 
*would* allow us to issue a warning here.

Second, the other warning is still useful. If users start to see stalls in 
their consumer, they may start looking at consumer logs, but eventually it will 
make sense to look at the brokers as well. And for the replication failure, 
they're likely to look at all the brokers for the given partition. Providing 
help *anywhere* is much better than the current situation of just silently 
stalling.


- Ewen Cheslack-Postava


On July 21, 2015, 4:21 p.m., Edward Ribeiro wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/36578/
 ---
 
 (Updated July 21, 2015, 4:21 p.m.)
 
 
 Review request for kafka.
 
 
 Bugs: KAFKA-2338
 https://issues.apache.org/jira/browse/KAFKA-2338
 
 
 Repository: kafka
 
 
 Description
 ---
 
 KAFKA-2338 Warn users if they change max.message.bytes that they also need to 
 update broker and consumer settings
 
 
 Diffs
 -
 
   core/src/main/scala/kafka/admin/TopicCommand.scala 
 4e28bf1c08414e8e96e6ca639b927d51bfeb4616 
 
 Diff: https://reviews.apache.org/r/36578/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Edward Ribeiro
 

Re: Review Request 36578: Patch for KAFKA-2338

[Ewen Cheslack-Postava]

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


In addition to inline comments, the patch fell out of sync recently and will 
need a rebase.


core/src/main/scala/kafka/admin/TopicCommand.scala (line 90)
https://reviews.apache.org/r/36578/#comment146503

This format call isn't working because it's being called on the second 
string due to the split across lines and +. Needs parens added or switched back 
to a single string.



core/src/main/scala/kafka/server/AbstractFetcherThread.scala (line 166)
https://reviews.apache.org/r/36578/#comment146524

My quick test indicates this doesn't work -- I don't think this is the code 
that would be returned anyway since I don't think max message size is checked 
for fetch requests, fetch.message.max.bytes for consumers and  
replica.fetch.max.bytes for brokers are both for aggregate data size. The 
problem occurs when the total aggregate size permitted per request is smaller 
than a single message. I think in that case we're just returning 0 messages, 
but not an error code. After enough time the result is that the ISR shrinks 
(which is what my test showed in the broker logs).

I think to properly log this we might need to log it on the leader node not 
on the fetcher -- probably somewhere in ReplicaManager. If you're not familiar 
with this code, you'll want to start in KafkaApis.scala in handleOffsetRequest. 
However, I'm not sure what this means about issuing a useful warning to 
consumers since they wouldn't have easy access to broker logs. On the other 
hand, a stalled consumer is less of a problem than the ISR being forced down to 
a single replica.

For reference, I tested this with a simple config with two brokers, 
adjusting message.max.bytes and replica.fetch.max.bytes. Then I created a topic 
with replication factor 2 and used console producer to send data of different 
sizes to test the output.


- Ewen Cheslack-Postava


On July 18, 2015, 3:37 a.m., Edward Ribeiro wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/36578/
 ---
 
 (Updated July 18, 2015, 3:37 a.m.)
 
 
 Review request for kafka.
 
 
 Bugs: KAFKA-2338
 https://issues.apache.org/jira/browse/KAFKA-2338
 
 
 Repository: kafka
 
 
 Description
 ---
 
 KAFKA-2338 Warn users if they change max.message.bytes that they also need to 
 update broker and consumer settings
 
 
 Diffs
 -
 
   core/src/main/scala/kafka/admin/TopicCommand.scala 
 a90aa8787ff21b963765a547980154363c1c93c6 
   core/src/main/scala/kafka/server/AbstractFetcherThread.scala 
 f84306143c43049e3aa44e42beaffe7eb2783163 
 
 Diff: https://reviews.apache.org/r/36578/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Edward Ribeiro
 

Review Request 36578: Patch for KAFKA-2338

[Edward Ribeiro]

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

Review request for kafka.


Bugs: KAFKA-2338
https://issues.apache.org/jira/browse/KAFKA-2338


Repository: kafka


Description
---

KAFKA-2338 Warn users if they change max.message.bytes that they also need to 
update broker and consumer settings


Diffs
-

  core/src/main/scala/kafka/admin/TopicCommand.scala 
a90aa8787ff21b963765a547980154363c1c93c6 
  core/src/main/scala/kafka/server/AbstractFetcherThread.scala 
f84306143c43049e3aa44e42beaffe7eb2783163 

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


Testing
---


Thanks,

Edward Ribeiro

Re: Review Request 36578: Patch for KAFKA-2338

[Edward Ribeiro]

 On July 18, 2015, 12:10 a.m., Ashish Singh wrote:
  core/src/main/scala/kafka/admin/TopicCommand.scala, lines 56-62
  https://reviews.apache.org/r/36578/diff/1/?file=1014811#file1014811line56
 
  Nit: What changed here? Its always a good idea to keep non-functional 
  changes very minimal. If there a re a lot of them, they can be addressed in 
  a separate patch. Makes it easier to review functional changes.

You right. Gonna fix this one and unrelated changes below.


- Edward


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


On July 17, 2015, 7:32 p.m., Edward Ribeiro wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/36578/
 ---
 
 (Updated July 17, 2015, 7:32 p.m.)
 
 
 Review request for kafka.
 
 
 Bugs: KAFKA-2338
 https://issues.apache.org/jira/browse/KAFKA-2338
 
 
 Repository: kafka
 
 
 Description
 ---
 
 KAFKA-2338 Warn users if they change max.message.bytes that they also need to 
 update broker and consumer settings
 
 
 Diffs
 -
 
   core/src/main/scala/kafka/admin/TopicCommand.scala 
 a90aa8787ff21b963765a547980154363c1c93c6 
   core/src/main/scala/kafka/server/AbstractFetcherThread.scala 
 f84306143c43049e3aa44e42beaffe7eb2783163 
 
 Diff: https://reviews.apache.org/r/36578/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Edward Ribeiro
 

Re: Review Request 36578: Patch for KAFKA-2338

[Edward Ribeiro]

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

(Updated July 18, 2015, 3:37 a.m.)


Review request for kafka.


Bugs: KAFKA-2338
https://issues.apache.org/jira/browse/KAFKA-2338


Repository: kafka


Description
---

KAFKA-2338 Warn users if they change max.message.bytes that they also need to 
update broker and consumer settings


Diffs (updated)
-

  core/src/main/scala/kafka/admin/TopicCommand.scala 
a90aa8787ff21b963765a547980154363c1c93c6 
  core/src/main/scala/kafka/server/AbstractFetcherThread.scala 
f84306143c43049e3aa44e42beaffe7eb2783163 

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


Testing
---


Thanks,

Edward Ribeiro

Re: Review Request 36578: Patch for KAFKA-2338

[Edward Ribeiro]

 On July 18, 2015, 12:10 a.m., Ashish Singh wrote:
  core/src/main/scala/kafka/admin/TopicCommand.scala, line 87
  https://reviews.apache.org/r/36578/diff/1/?file=1014811#file1014811line87
 
  I guess it is unnecessary to parse 0 as int. You can have 
  maxMessageSize init to 0 and update if getProperty returns a non null value.

Okay, but doing so it will be something like:

```
var maxMessageSize = 0
if (configs.getProperty(LogConfig.MaxMessageBytesProp) != null) 
   maxMessageSize = 
Integer.parseInt(configs.getProperty(LogConfig.MaxMessageBytesProp))
```

The second case still has to parseInt() a String, it uses a ``var``, and spends 
more lines (three or four, if I create a second variable to hold 
getProperty()). I don't see a particular advantage in doing this tbh, but I am 
fine with either way.


- Edward


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


On July 17, 2015, 7:32 p.m., Edward Ribeiro wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/36578/
 ---
 
 (Updated July 17, 2015, 7:32 p.m.)
 
 
 Review request for kafka.
 
 
 Bugs: KAFKA-2338
 https://issues.apache.org/jira/browse/KAFKA-2338
 
 
 Repository: kafka
 
 
 Description
 ---
 
 KAFKA-2338 Warn users if they change max.message.bytes that they also need to 
 update broker and consumer settings
 
 
 Diffs
 -
 
   core/src/main/scala/kafka/admin/TopicCommand.scala 
 a90aa8787ff21b963765a547980154363c1c93c6 
   core/src/main/scala/kafka/server/AbstractFetcherThread.scala 
 f84306143c43049e3aa44e42beaffe7eb2783163 
 
 Diff: https://reviews.apache.org/r/36578/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Edward Ribeiro
 

Re: Review Request 36578: Patch for KAFKA-2338

[Ashish Singh]

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



core/src/main/scala/kafka/admin/TopicCommand.scala (lines 56 - 62)
https://reviews.apache.org/r/36578/#comment146106

Nit: What changed here? Its always a good idea to keep non-functional 
changes very minimal. If there a re a lot of them, they can be addressed in a 
separate patch. Makes it easier to review functional changes.



core/src/main/scala/kafka/admin/TopicCommand.scala (line 87)
https://reviews.apache.org/r/36578/#comment146107

I guess it is unnecessary to parse 0 as int. You can have maxMessageSize 
init to 0 and update if getProperty returns a non null value.



core/src/main/scala/kafka/admin/TopicCommand.scala (line 119)
https://reviews.apache.org/r/36578/#comment146108

Non-func change?



core/src/main/scala/kafka/admin/TopicCommand.scala (line 129)
https://reviews.apache.org/r/36578/#comment146109

Non-func change?



core/src/main/scala/kafka/admin/TopicCommand.scala (line 145)
https://reviews.apache.org/r/36578/#comment146110

Non-func change?


- Ashish Singh


On July 17, 2015, 7:32 p.m., Edward Ribeiro wrote:
 
 ---
 This is an automatically generated e-mail. To reply, visit:
 https://reviews.apache.org/r/36578/
 ---
 
 (Updated July 17, 2015, 7:32 p.m.)
 
 
 Review request for kafka.
 
 
 Bugs: KAFKA-2338
 https://issues.apache.org/jira/browse/KAFKA-2338
 
 
 Repository: kafka
 
 
 Description
 ---
 
 KAFKA-2338 Warn users if they change max.message.bytes that they also need to 
 update broker and consumer settings
 
 
 Diffs
 -
 
   core/src/main/scala/kafka/admin/TopicCommand.scala 
 a90aa8787ff21b963765a547980154363c1c93c6 
   core/src/main/scala/kafka/server/AbstractFetcherThread.scala 
 f84306143c43049e3aa44e42beaffe7eb2783163 
 
 Diff: https://reviews.apache.org/r/36578/diff/
 
 
 Testing
 ---
 
 
 Thanks,
 
 Edward Ribeiro