MINOR: Fixes version lookup exception.

[MicahZoltu]

Given a schema with 2 versions (0 and 1), if you pass in a version of 2 you will get an OutOfBoundsException instead of an IllegalArgumentException.

This fixes the problem by changing the check from > to >=, which will now return true in the given scenario.

[ijuma]

Thanks for the PR. Could you please add a test too?

[MicahZoltu]

Test added. Let me know if you want any additional tests.

[ijuma]

LGTM (assuming that tests pass).

[granthenke]

Instead of using length of the versions list, would it make sense to check if the value is greater than the latest version?

[MicahZoltu]

I don't have the full source open in front of me, is there a convenient getter for latest version available already? As far as performance, I believe an array length check is about as fast as it gets short of a constant value. For readability though I think version > versions.latest would be more clear if that is an available thing.

[granthenke]

This class, ProtoUtils, has public static short latestVersion(int apiKey)

[MicahZoltu]

That seems like a reasonable course of action:

if (version < 0 || version > latestVersion(apiKey) {
    throw ...
}

I'll try to make that change this evening.

[MicahZoltu]

Fixed.

[granthenke]

Can you change the title of the PR to include "MINOR: "?

Just a few small comments otherwise it looks good.

[MicahZoltu]

All outstanding issues addressed and rebased on latest trunk.

[granthenke]

LGTM

[ijuma]

@ewencp maybe you could review this simple fix?

[ewencp]

Hmm, seems like now we're doing a duplicate check on apiKey ranges. This doesn't seem critical since it's all really cheap, but it seems wasteful.

[MicahZoltu]

@ewencp Yeah, I noticed that when I switched over to using the latestVersion method. However, I believe that the readability/clarity of the code is worth the cost since the checks are incredibly cheap. We could rollback to what I had before if others disagree with that trade-off, in which we just compare against versions.length.

[ewencp]

@Zoltu Sure, that makes sense. LGTM.