KAFKA-2512: Add version check to broker and clients.#200
Conversation
|
kafka-trunk-git-pr #380 FAILURE |
There was a problem hiding this comment.
I think this can instead be:
@Test(expected = classOf[UnsupportedVersionException])
def testInvalidMagicByte() {
// Create a message
val message = new Message("value".getBytes, "key".getBytes, NoCompressionCodec)
message.buffer.put(Message.MagicOffset, (Message.CurrentMagicValue + 1).toByte)
message.buffer.putInt(Message.CrcOffset, (message.computeChecksum() & 0xffffffffL).toInt)
message.ensureValid()
}
|
kafka-trunk-git-pr #384 FAILURE |
|
The test failure seems not related. Ran the test again locally and it passed. |
|
This mostly looks good to me, but I think we're only testing the magic byte errors in this patch? What about a test for at least one or a couple of the KafkaApis checks for version errors? |
|
@ewencp Fair point about the unit test. I didn't write unit test before because it might be a bit hacky and I think the right way to do it is to add test after we have protocol version control on clients. I added unit test and did find an issue with new java class protocol. The test itself is a bit hacky but should be good for now. We need to write better test after we have client protocol version control. I'll create a ticket to track this. |
|
kafka-trunk-git-pr #422 SUCCESS |
|
@becketqin Is this already incorporated in your timestamp patch? |
|
If it's not, it needs to be rebased. |
|
It would be nice to rebase this. |
| //TODO: this method should only use request.header after all the requests are migrated to use client java request class. | ||
| // For the requests using old scala class, we need to pass in the API version explicitly because the Request.header will | ||
| // be null. For requests using new java class. the API version will be None. | ||
| def validateRequestVersion(request: RequestChannel.Request, apiVersion: Option[Short]) { |
There was a problem hiding this comment.
Can this all be handled in RequestChannel or before the big match statement in KafkaApis so we are guaranteed to have all messages checked?
There was a problem hiding this comment.
I agree that it would be better if we can do it that way.
|
@granthenke @ijuma @guozhangwang Sorry for the belated response, I just saw your comments. I will rebase the patch. The version check on this patch makes sense, but the returned unsupported version might will only work for the responses that have an error code field. With KIP-35 a well behaved client should not send unsupported API version to the broker. But from the broker's point of view, we probably still need to handle this in case there are some problematic clients. I will rebase this and update the PR. Thanks. |
|
@becketqin Will you have a chance to rebase this soon? |
10 participants
No description provided.