KAFKA-3307: Add ApiVersions Request/Response and server side handling. by SinghAsDev · Pull Request #986 · apache/kafka

SinghAsDev · 2016-02-29T23:35:04Z

The patch does the following.

Adds ApiVersionsRequest/Response.
Adds UNSUPPORTED_VERSION error and UnsupportedVersionException.
Adds broker side handling of ApiVersionsRequest.

granthenke · 2016-03-01T17:59:12Z

+
+    public static final Schema SINGLE_PROTOCOL_VERSION_V0 = new Schema(
+        new Field("api_key", INT16, "Api key of protocol."),
+        new Field("api_name", STRING, "Api name of protocol."),


Do we need the name field? How would it be used?

It has the human readable api name. Depending on use case it might be handy.

SinghAsDev · 2016-04-21T21:17:54Z

@gwenshap @edenhill @hachikuji mind reviewing this?

hachikuji · 2016-04-21T23:33:39Z

Is this right? Doesn't this mean that cluster Describe access needs to be given to all clients?

It is usually treated as security flaw to be able to know what version is supported by server, as with that knowledge one can exploit security flaws associated with the version. Does cluster describe seem too restrictive? Trying to think what else can be done here. Do you have any suggestion?

I think the security gain from restricting this API is a little dubious, but I can understand the desire to limit information leakage. Anyway, perhaps there's no need to rehash the argument since the KIP already says this?

The broker returns its full list of supported ApiKeys and versions regardless of current authentication state (e.g., before SASL authentication). If this is considered to leak information SSL can be used for early authentication.

Agreed. Updating it.

gwenshap · 2016-04-22T22:38:00Z

Do you mind also generating protocol docs and making sure the new protocol is documented correctly?

SinghAsDev · 2016-04-23T20:44:20Z

@gwenshap, I have verified that new api and error shows up on generated docs.

gwenshap · 2016-04-25T01:51:46Z

I would prefer to make it a bit more generic (i.e. UnknownApiVersion or UnknownRequestVersion or something like that)
This will allow us to reuse this error for other Requests in the future.

Do you see any reason not to?

We should coordinate this with @becketqin. He has a pull request that validates the versions for all other requests and it adds UnsupportedVersionException: #200

Yes, let's please try to use concise and descriptive names. UnsupportedVersionException sounds good to me, personally.

Makes sense. I will rename this to UnsupportedVersionException.

gwenshap · 2016-04-26T00:56:34Z

@SinghAsDev - ApiVersion and ApiVersions is a tiny change and no one is using the API yet (since we didn't commit yet...), I see no problems. Whatever you see fit is fine by me :)

edenhill · 2016-04-26T00:56:43Z

Go ahead with renaming it.

ijuma · 2016-04-26T19:36:02Z

I would remove this and the 4th constructor as it's good for a message including some more information to be included.

Makes sense, in fact do you think it makes sense to have a follow up JIRA to do this exercise for all exceptions? One can argue that it is a public class in common, but I do not think it should be too big of a deal. If you agree, I can file a follow up JIRA and take on that in some time.

As you said, the errors package is public so we cannot change it.

ijuma · 2016-04-26T19:39:20Z

@SinghAsDev, can you please update the PR title and description to match what's actually in the PR?

SinghAsDev · 2016-04-27T00:30:50Z

@ijuma I believe all review comments are addressed now. Mind taking a look again?

ijuma · 2016-04-27T00:32:03Z

@SinghAsDev, can you please merge trunk into this branch as it no longer merges cleanly.

…quest version is greater than supported ApiRequest version

1. Rename Errors.UNKNOWN_API_VERSION_REQUEST_VERSION => Errors.UNSUPPORTED_VERSION. 2. Use JUnit.assert instead of scala's assert in tests. 3. Maintain map of ApiKey to ApiVersion in ApiVersionResponse. 4. Add note in KafkaApis on why authentication is not performed while handling ApiVersionRequest

…equest and ApiVersionResponse => ApiVersionsResponse

force adding some useful info while throwing exception.

SinghAsDev · 2016-04-27T00:52:35Z

@ijuma done.

ijuma · 2016-04-27T00:54:31Z

-    SASL_HANDSHAKE(17, "SaslHandshake");
+    SASL_HANDSHAKE(17, "SaslHandshake"),
+    API_VERSION(18, "ApiVersion"),
+    API_VERSIONS(17, "ApiVersions");


I think something went wrong here.

SinghAsDev · 2016-04-27T05:18:38Z

@ijuma seems like I messed up while rebasing, should be fixed now. Thanks for the reviews!

SinghAsDev · 2016-04-27T17:25:10Z

@ijuma does this look good now?

ijuma · 2016-04-27T17:31:00Z

+      val version = KafkaApis.apiVersionsResponse.apiVersion(key.id)
+      assertNotNull("Could not find ApiVersion for API " + key.name, version)
+      assertEquals("Incorrect min version for Api " + key.name, version.minVersion, Protocol.MIN_VERSIONS(key.id))
+      assertEquals("Incorrect min version for Api " + key.name, version.maxVersion, Protocol.CURR_VERSION(key.id))


The message seems wrong here. Can you also please use string interpolation instead of concatenation (there are other cases in this file).

ijuma · 2016-04-27T17:34:21Z

@SinghAsDev, looks good apart from one minor comment.

SinghAsDev · 2016-04-27T17:59:22Z

@ijuma made the change, built and ran tests locally.

ijuma · 2016-04-27T18:00:56Z

LGTM

gwenshap · 2016-04-27T18:27:32Z

LGTM. Thanks for the contribution!

The patch does the following. 1. Adds ApiVersionsRequest/Response. 2. Adds UNSUPPORTED_VERSION error and UnsupportedVersionException. 3. Adds broker side handling of ApiVersionsRequest. Author: Ashish Singh <asingh@cloudera.com> Reviewers: Gwen Shapira, Ismael Juma, Magnus Edenhill Closes apache#986 from SinghAsDev/KAFKA-3307

SinghAsDev changed the title ~~Kafka 3307: Add ProtocolVersion request/response and server side handling.~~ KAFKA-3307: Add ProtocolVersion request/response and server side handling. Feb 29, 2016

SinghAsDev closed this Feb 29, 2016

SinghAsDev reopened this Feb 29, 2016

granthenke reviewed Mar 1, 2016
View reviewed changes

SinghAsDev force-pushed the KAFKA-3307 branch from a26bc6a to 007eb83 Compare April 21, 2016 20:43

SinghAsDev mentioned this pull request Apr 21, 2016

KAFKA-3600: Enhance java clients to use ApiVersion Req/Resp to check if the broker they are talking to supports required api versions #1251

Closed

hachikuji reviewed Apr 21, 2016
View reviewed changes

SinghAsDev force-pushed the KAFKA-3307 branch from b5f5f3c to 6501f49 Compare April 23, 2016 20:27

gwenshap reviewed Apr 25, 2016
View reviewed changes

SinghAsDev mentioned this pull request Apr 26, 2016

KAFKA-2512: Add version check to broker and clients. #200

Closed

ijuma reviewed Apr 26, 2016
View reviewed changes

SinghAsDev changed the title ~~KAFKA-3307: Add ProtocolVersion request/response and server side handling.~~ KAFKA-3307: Add ApiVersions Request/Response and server side handling. Apr 26, 2016

Ashish Singh added 6 commits April 26, 2016 17:44

KAFKA-3307: Add ApiVersion request/response and server side handling.

f28f40e

Remove check for cluster describe privilege for api version

917d8d0

Throw UNKNOWN_API_VERSION_REQUEST_VERSION exception when ApiVersionRe…

6ae3102

…quest version is greater than supported ApiRequest version

Rename API_VERSION => API_VERSIONS, ApiVersionRequest => ApiVersionsR…

9fae93f

…equest and ApiVersionResponse => ApiVersionsResponse

Remove a few constructor overloads from UnsupportedVersionException to

1aaf5aa

force adding some useful info while throwing exception.

SinghAsDev force-pushed the KAFKA-3307 branch from d765acc to 1aaf5aa Compare April 27, 2016 00:52

ijuma reviewed Apr 27, 2016
View reviewed changes

Fix rebase errors.

34039c3

ijuma reviewed Apr 27, 2016
View reviewed changes

Use string interpolation and fix typo in assert message.

f3c3402

asfgit closed this in 8407dac Apr 27, 2016

Conversation

SinghAsDev commented Feb 29, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SinghAsDev commented Apr 21, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gwenshap commented Apr 22, 2016

Uh oh!

SinghAsDev commented Apr 23, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gwenshap commented Apr 26, 2016

Uh oh!

edenhill commented Apr 26, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ijuma commented Apr 26, 2016

Uh oh!

SinghAsDev commented Apr 27, 2016

Uh oh!

ijuma commented Apr 27, 2016

Uh oh!

SinghAsDev commented Apr 27, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SinghAsDev commented Apr 27, 2016

Uh oh!

SinghAsDev commented Apr 27, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ijuma commented Apr 27, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SinghAsDev commented Apr 27, 2016

Uh oh!

ijuma commented Apr 27, 2016

Uh oh!

gwenshap commented Apr 27, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

SinghAsDev commented Feb 29, 2016 •

edited

Loading

ijuma commented Apr 27, 2016 •

edited

Loading