KAFKA-5919: Adding checks on "version" field for tools using it by ppatierno · Pull Request #3887 · apache/kafka

ppatierno · 2017-09-18T09:51:36Z

Removed ignored "version" field in JSON file for deleting records

tedyu · 2017-09-18T18:23:40Z

lgtm

ppatierno · 2017-09-20T06:59:57Z

Retest this please

ijuma · 2017-09-28T12:19:38Z

cc @lindong28

lindong28 · 2017-09-28T12:32:51Z

@ppatiern I think it is useful to have it in the json file for the same reason we have it in e.g. ReassignPartitionsCommand. It can be useful if in the future we have to make backward incompatible change in the format of the json file provided to the DeleteRecordsCommand.

If it is confusing that the version field is not used now, maybe we can add code to throw exception if version is not 1, and add comment to the version field to explain why we have it. What do you think?

ppatierno · 2017-09-28T12:54:36Z

@lindong28 in the ReassignPartitionsCommand, the JSON file can be generated and then modified in order to be provided to the same tool as well. The "generate" option writes "version":1 into the JSON (it's hard coded). I think that the first thing could be making this field mandatory, so checking that it exists in the parsed JSON and then checking that it's 1 (for now). Of course adding a comment on why we have it is really useful for the user. Wdyt ?

lindong28 · 2017-09-28T13:03:54Z

@ppatierno I did a quick search in ReassignPartitionsCommand.scala and it seems that the version filed is not checked when the json file is parsed. So strictly speaking that version field is not currently mandatory for ReassignPartitionsCommand because we can provide a json file without version field to ReassignPartitionsCommand.

I agreed that it is more obvious to user that version field is needed for ReassignPartitionsCommand because it is included in the generated json filed. But I think the actual reason we put version filed there is because we may need it in the future when we make backward incompatible change -- otherwise we don't even need to include it in the generated json file.

The need to prepare for potential backward incompatible change is shared between both ReassignPartitionsCommand and DeleteRecordsCommand. So I think for consistency reason we should either keep the version field in json file for all such tools, or remove it from all such tools. I think it is useful to have it there in general. Does this make sense?

ppatierno · 2017-09-28T13:11:43Z

@lindong28 yes, in fact my checking proposal would have been extended to the ReassignPartitionsCommand tool as well because as the DeleteRecordsCommand doesn't care about the "version" field. I'm with you about your concerns on compatibility but if we want to keep this field for both tools I think that it should be mandatory so a check on it could be needed when parsing the JSON file otherwise the user could be confused about that field. What do you think about this checking ? Or do you prefer not having it as mandatory but just adding a comment line for the user ? My opinion is that having a consistent JSON file is better so checking that the "version" field is there could be good.

lindong28 · 2017-09-28T13:32:16Z

@ppatierno I would prefer to check for the value of version field in both ReassignPartitionsCommand and DeleteRecordsCommand (any other tools if any). The version field defaults to 1 if it is not explicitly specified in the json file. As of now, the only valid value of version field for both tools is 1. If user specified any value different than 1, the tool should throw exception and exit.

By doing this we will keep the behavior backward compatible. I assume no (or very few) user will explicitly specify version field that is different than 1 in the json file. And we retain the capability to evolve the format of json file in the future by having this version filed.

ppatierno · 2017-09-28T13:35:56Z

@lindong28 for sure I want to check on the value but even checking that the "version" field is there. I mean : if user provides a JSON file without "version" field, current application works well. I want to check that the field is provided and then it's also 1. Or are you saying that if the field isn't there it's ok (we consider 1 as default) but if it's there we have to check that its value is 1 ?

lindong28 · 2017-09-28T13:38:39Z

@ppatierno Yes, that is what I am saying. If user does not specify version field, we assume the version is 1. Then we check whether the value of the version field is supported.

ppatierno · 2017-09-28T13:55:36Z

@lindong28 it's just my opinion but a field like "version" that has its weight because it's useful for compatibility check should have a consistent usage. If you don't put it into the JSON file you are saying : "this is my JSON file with this schema, please try to handle it". Is your idea that in the future we could support different versions in the same tool ? Without providing it the tool consider its current supported version and try to verify the JSON file that could be wrong. Making version field mandatory could be a way for avoiding this and having the tool first checking the version and raising an exception immediately if it can't support it, without going ahead and discovering that one of the new fields in the JSON is missing for example.

ppatierno · 2017-09-28T14:05:17Z

@lindong28 for sure having it mandatory is a breaking change and a KIP would be needed. Maybe it doesn't make sense to have it mandatory even for this reason ?

lindong28 · 2017-09-28T14:13:56Z

Yes, the idea is that we could support different version in the same tool. And yes, if we were to make ti mandatory, it may potentially break existing usage and a KIP is needed.

In my opinion, if user does not specify it, the tool considers the version to be the earlier the version rather than the latest version. By doing this, it is unlikely that "Without providing it the tool consider its current supported version and try to verify the JSON file that could be wrong", right?

ppatierno · 2017-09-28T14:16:13Z

@lindong28 ah ok ... I got it in the wrong way that without version, the tool uses the latest. So finally I agree with you. I'll update this PR and will open a new JIRA and PR for the ReassignPartitionsCommand tool as well. Thanks for your patience ;)

lindong28 · 2017-09-28T14:18:56Z

@ppatierno Sure thing. Thanks for taking time to improve these tools.

ppatierno · 2017-09-28T16:41:33Z

@ijuma in my understanding about the release plan we are still on time to have these changes in the 1.0.0 release right ? I need to do that before October 4th and giving you time for the review. The related JIRA is still marked as fixed for 1.0.0 release.

ppatierno · 2017-09-29T07:52:38Z

@lindong28 what do you think about this first iteration of changes on checking version and JSON content ?

lindong28

Thanks for the update. A few comment below.

lindong28 · 2017-09-29T14:42:49Z

It seems that the logic of throwing exception with unsupported version is duplicated here and in parseJsonData(). Maybe we can simplify the patch by removing the logic of checking supportedVersions and remove the variable supportedVersions?

You are absolutely right !

lindong28 · 2017-09-29T14:47:51Z

I think the logic here is to parse data based on the supported version. In this case the supported version is 1. It does not really matter whether it is earliest version or not, right? This can be similar to org.apache.kafka.common.requests.ProduceRequest.requiredMagicForVersion.

If I got this right the your point was why I was using the EarliestVersion and not simple 1. I was just trying to avoid a literal here because our first supported version is the "earliest" one but maybe in this case it makes much more sense.

lindong28 · 2017-09-29T14:51:07Z

I am not sure we need to throw exception here. Can we keep the existing behavior and return an empty sequence in this case?

Isn't it better give to the user a reason why no records are processed ? The current output on the console is :

Executing records delete operation
Records delete operation completed:

Just no records processed but a clear reason is not provided

According to the Java doc of parseFull(), it seems that parseFull() returns None only if the input string is not valid JSON, right? If so, one more minor comment is that the error message can be something like the input string is not a valid json.

lindong28 · 2017-09-29T16:16:33Z

Thanks for the update. I have one more minor comment. Otherwise LGTM.

ppatierno · 2017-09-29T16:22:28Z

I made latest change. Thanks @lindong28 !!
I'll open a similar JIRA around these version checks with PreferredReplicaLeaderElectionCommand tool asking to you to take a look as well ;)
Maybe @ijuma we can merge this PR for the coming release.

lindong28 · 2017-09-30T00:54:39Z

@ppatierno Given that the purpose of checking version field is same among all these tools, I actually think it may be simpler to put them in the same pull request instead of having 3+ pull request.. This simplifies the work for committer.

ppatierno · 2017-10-02T08:01:50Z

@lindong28 ok, I'm going to do the same for the ReassignPartitionsCommand tool and updating the related JIRA. My only doubt is that you are speaking about 3+ pull requests as there are 3+ tools which have a JSON file in input with a "version" field. I can only see the DeleteRecordsCommand and the ReassignPartitionsCommand. The LogDirsCommand for example just generates the file but hasn't an input like that.

lindong28 · 2017-10-02T09:52:56Z

@ppatierno I think LogDirsCommand.formatAsJson() also generates a version field. This it is implicitly expected but not explicitly checked, similar to DeleteRecordsCommand. I guess my idea is that the change needed to check the version field is very similar across all tools and is small in size. So it is not very worthwhile to have multiple commits for this. Just my personal opinion. Give that the concept is clear, we probably one need only one committer to review your patch. That committer can comment on this.

ppatierno · 2017-10-02T09:58:36Z

@lindong28 as far as I understood the LogDirsCommand just generates a JSON output with this field. Such JSON file isn't used as input in order to check the version field which is what I'm working on regarding the other two tools.
Yes I think that it could be better having a committer reviewing this; I have started to ping you because @ijuma "ccing" you on my first commit and I really appreciate your feedback and thanks for the time you are spending on doing this.

lindong28 · 2017-10-02T10:01:51Z

@ppatierno Ah you are right. LogDirsCommand does not read any json file. Then we only need to change two tools .Thanks for the clarification!

ppatierno · 2017-10-19T09:20:54Z

@ijuma now that we have branch for 1.0, can you take a look to this for the trunk ? I have applied changes requested by @lindong28 who did a great review. Thanks !

implements KIP-303 Reviewers: Bill Bejeck <bill@confluent.io>, John Roesler <john@confluent.io>, Matthias J. Sax <matthias@confluent.io>

This commit allows secrets in Connect configs to be externalized and replaced with variable references of the form `${provider:[path:]key}`, where the "path" is optional. There are 2 main additions to `org.apache.kafka.common.config`: a `ConfigProvider` and a `ConfigTransformer`. The `ConfigProvider` is an interface that allows key-value pairs to be provided by an external source for a given "path". An a TTL can be associated with the key-value pairs returned from the path. The `ConfigTransformer` will use instances of `ConfigProvider` to replace variable references in a set of configuration values. In the Connect framework, `ConfigProvider` classes can be specified in the worker config, and then variable references can be used in the connector config. In addition, the herder can be configured to restart connectors (or not) based on the TTL returned from a `ConfigProvider`. The main class that performs restarts and transformations is `WorkerConfigTransformer`. Finally, a `configs()` method has been added to both `SourceTaskContext` and `SinkTaskContext`. This allows connectors to get configs with variables replaced by the latest values from instances of `ConfigProvider`. Most of the other changes in the Connect framework are threading various objects through classes to enable the above functionality. Author: Robert Yokota <rayokota@gmail.com> Author: Ewen Cheslack-Postava <me@ewencp.org> Reviewers: Randall Hauch <rhauch@gmail.com>, Ewen Cheslack-Postava <ewen@confluent.io> Closes apache#5068 from rayokota/KAFKA-6886-connect-secrets

This patch contains a few follow-up improvements/cleanup for KIP-266: - Add upgrade notes - Add missing `commitSync(Duration)` API - Improve timeout messages and fix some naming inconsistencies - Various small cleanups Reviewers: John Roesler <john@confluent.io>, Guozhang Wang <wangguoz@gmail.com>

…4636) implements KIP-268 Reviewers: Bill Bejeck <bill@confluent.io>, John Roesler <john@confluent.io>, Guozhang Wang <guozhang@confluent.io>

…n broker [KIP-283] (apache#4871) Implementation for lazy down-conversion in a chunked manner for efficient memory usage during down-conversion. This pull request is mainly to get initial feedback on the direction of the patch. The patch includes all the main components from KIP-283. Reviewers: Jason Gustafson <jason@confluent.io>

@guozhangwang

…ache#5066) In apache#4919 we propagate the SerDes for each of these aggregation operators. As @guozhangwang mentioned in that PR: ``` reduce: inherit the key and value serdes from the parent XXImpl class. count: inherit the key serdes, enforce setting the Serdes.Long() for value serdes. aggregate: inherit the key serdes, do not set for value serdes internally. ``` Although it's all good for reduce and count, it is quiet unsafe to have aggregate without Materialized given. In fact I don't see why we would not give a Materialized for the aggregate since the result type will always be different (otherwise use reduce) and also the value Serde is simply not propagated. This has been discussed previously in a broader PR before but I believe for aggregate we could pass implicitly a Materialized the same way we pass a Joined, just to avoid the stupid case. Then if the user wants to specialize, he can give his own Materialized. Reviewers: Debasish Ghosh <dghosh@acm.org>, Guozhang Wang <guozhang@confluent.io>

ppatierno · 2018-06-01T10:43:00Z

@lindong28 as you said this is a nice to have patch. Now that we are closed to the 2.0.0, is there still interest on that if I fix the conflicts that there are today ? It doesn't have a related KIP but I think that it wasn't needed for the type of change.

Specifying an invalid config (i.e. something other than `CreateTime` or `LogAppendTime`) via `TopicCommand` would previously cause the broker to fail on start-up. Reviewers: Manikumar Reddy <manikumar.reddy@gmail.com>, Ismael Juma <ismael@juma.me.uk>

lindong28 · 2018-06-01T17:22:50Z

@ppatierno Certainly. I will review the patch after you rebase it. Thanks!

Changes to keep the operation name as is and make the sensor name unique. Reviewers: John Roesler <john@confluent.io>, Matthias J. Sax <matthias@confluent.io>, Guozhang Wang <wangguoz@gmail.com>

- Override toString in LeaderAndIsrResponse and StopReplicaResponse - Add unit tests Reviewers: Ismael Juma <ismael@juma.me.uk>

Added more checks on the JSON data file

…kafka-5919

ppatierno · 2018-06-02T13:08:04Z

@lindong28 I had some problems rebasing but using merge now it seems that the PR is really huge while the files involved are just two : ReassignPartitionsCommand and DeleteRecordsCommand.
Sometimes git is a pain. How we can deal with this now ? :-(

ppatierno · 2018-06-02T13:17:51Z

@lindong28 what do you think if I create a new branch from current trunk and then just override contents from ReassignPartitionsCommand and DeleteRecordsCommand opening a new PR ?

ppatierno · 2018-06-02T13:25:59Z

@lindong28 see new PR #5126

Adding checks on "version" field for tools using it. This is a new version of the closed PR #3887 (to see for more comments and related discussion). Author: Paolo Patierno <ppatierno@live.com> Reviewers: Dong Lin <lindong28@gmail.com> Closes #5126 from ppatierno/kafka-5919-update

Adding checks on "version" field for tools using it. This is a new version of the closed PR apache#3887 (to see for more comments and related discussion). Author: Paolo Patierno <ppatierno@live.com> Reviewers: Dong Lin <lindong28@gmail.com> Closes apache#5126 from ppatierno/kafka-5919-update

ppatierno changed the title ~~KAFKA-5919: Delete records command "version" parameter ignored~~ KAFKA-5919: Adding checks on "version" field for delete records command Sep 29, 2017

ppatierno mentioned this pull request Sep 29, 2017

KAFKA-5940: Throw exception if the JSON file is invalid #3920

Closed

lindong28 reviewed Sep 29, 2017

View reviewed changes

ppatierno changed the title ~~KAFKA-5919: Adding checks on "version" field for delete records command~~ KAFKA-5919: Adding checks on "version" field for tools using it Oct 2, 2017

ppatierno force-pushed the kafka-5919 branch from 7fa1864 to f335886 Compare October 19, 2017 09:16

guozhangwang and others added 6 commits May 30, 2018 11:54

KAFKA-4936: Add dynamic routing in Streams (apache#5018)

f33e9a3

implements KIP-303 Reviewers: Bill Bejeck <bill@confluent.io>, John Roesler <john@confluent.io>, Matthias J. Sax <matthias@confluent.io>

KAFKA-6054: Add 'version probing' to Kafka Streams rebalance (apache#…

d166485

…4636) implements KIP-268 Reviewers: Bill Bejeck <bill@confluent.io>, John Roesler <john@confluent.io>, Guozhang Wang <guozhang@confluent.io>

emmanuelharel and others added 2 commits June 1, 2018 12:09

MINOR:Fix table outer join test (apache#5099)

e24916a

lindong28 self-assigned this Jun 1, 2018

bbejeck and others added 10 commits June 1, 2018 14:48

MINOR: Use thread name and task for sensor name (apache#5111)

cb2f024

Changes to keep the operation name as is and make the sensor name unique. Reviewers: John Roesler <john@confluent.io>, Matthias J. Sax <matthias@confluent.io>, Guozhang Wang <wangguoz@gmail.com>

KAFKA-6760: Fix response logging in the Controller (apache#4834)

73e2cbb

- Override toString in LeaderAndIsrResponse and StopReplicaResponse - Add unit tests Reviewers: Ismael Juma <ismael@juma.me.uk>

Removed ignored "version" field in JSON file for deleting records

f4783ca

Added handling default earliest version (1)

c777bd1

Added more checks on the JSON data file

Redadded "version" field in the documentation about JSON file format

23ca324

Added more one check on "partitions" field

26b96a6

Removed duplicated check and exception on not supported version

467b205

Changed the error message when the file content is not a valid JSON

08756fb

Added checking "version" field for the topics-to-move JSON input

463bf40

Merge branch 'kafka-5919' of https://github.com/ppatierno/kafka into …

d873ebe

…kafka-5919

ppatierno closed this Jun 2, 2018

ppatierno mentioned this pull request Jun 2, 2018

KAFKA-5919: Adding checks on "version" field for tools using it #5126

Closed

3 tasks

ppatierno deleted the kafka-5919 branch June 6, 2018 06:45

Uh oh!

Conversation

ppatierno commented Sep 18, 2017

Uh oh!

tedyu commented Sep 18, 2017

Uh oh!

ppatierno commented Sep 20, 2017

Uh oh!

ijuma commented Sep 28, 2017

Uh oh!

lindong28 commented Sep 28, 2017

Uh oh!

ppatierno commented Sep 28, 2017

Uh oh!

lindong28 commented Sep 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ppatierno commented Sep 28, 2017

Uh oh!

lindong28 commented Sep 28, 2017

Uh oh!

ppatierno commented Sep 28, 2017

Uh oh!

lindong28 commented Sep 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ppatierno commented Sep 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ppatierno commented Sep 28, 2017

Uh oh!

lindong28 commented Sep 28, 2017

Uh oh!

ppatierno commented Sep 28, 2017

Uh oh!

lindong28 commented Sep 28, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ppatierno commented Sep 28, 2017

Uh oh!

ppatierno commented Sep 29, 2017

Uh oh!

lindong28 left a comment

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!

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!

Choose a reason for hiding this comment

Uh oh!

lindong28 commented Sep 29, 2017

Uh oh!

ppatierno commented Sep 29, 2017

Uh oh!

lindong28 commented Sep 30, 2017

Uh oh!

ppatierno commented Oct 2, 2017

Uh oh!

lindong28 commented Oct 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ppatierno commented Oct 2, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

lindong28 commented Oct 2, 2017

Uh oh!

ppatierno commented Oct 19, 2017

Uh oh!

ppatierno commented Jun 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

lindong28 commented Sep 28, 2017 •

edited

Loading

lindong28 commented Sep 28, 2017 •

edited

Loading

ppatierno commented Sep 28, 2017 •

edited

Loading

lindong28 commented Sep 28, 2017 •

edited

Loading

lindong28 commented Oct 2, 2017 •

edited

Loading

ppatierno commented Oct 2, 2017 •

edited

Loading

ppatierno commented Jun 1, 2018 •

edited

Loading