Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

KAFKA-4914: Partition re-assignment tool should check types before pe… #2708

Merged
merged 12 commits into from Mar 31, 2018

Conversation

@nicktrav
Copy link
Contributor

commented Mar 19, 2017

…rsisting state in ZooKeeper

Prior to this, there have been instances where invalid data was allowed to be persisted in ZooKeeper, which causes ClassCastExceptions when a broker is restarted and reads this type-unsafe data.

Adds basic structural and type validation for the reassignment JSON via introduction of Scala case classes that map to the expected JSON structure.

@asfbot

This comment has been minimized.

Copy link

commented Mar 20, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/2269/
Test PASSed (JDK 8 and Scala 2.11).

@asfbot

This comment has been minimized.

Copy link

commented Mar 20, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/2266/
Test PASSed (JDK 7 and Scala 2.10).

@asfbot

This comment has been minimized.

Copy link

commented Mar 20, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/2266/
Test PASSed (JDK 8 and Scala 2.12).

@nicktrav

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2017

Hey @ijuma - would you mind taking a look? I see you've worked on this previously.

@nicktrav

This comment has been minimized.

Copy link
Contributor Author

commented Apr 14, 2017

@ewencp @ijuma - any chance of a review on this one?

@asfbot

This comment has been minimized.

Copy link

commented Apr 14, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/2952/
Test PASSed (JDK 8 and Scala 2.11).

@asfbot

This comment has been minimized.

Copy link

commented Apr 14, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.10/2947/
Test PASSed (JDK 7 and Scala 2.10).

@asfbot

This comment has been minimized.

Copy link

commented Apr 14, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/2948/
Test PASSed (JDK 8 and Scala 2.12).

@omkreddy

This comment has been minimized.

Copy link
Contributor

commented Jul 19, 2017

@nicktrav Thanks for the patch. LGTM. We also faced this issue in production. Can you rebase the PR?

@asfgit

This comment has been minimized.

Copy link

commented Jul 19, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.11/6169/
Test FAILed (JDK 7 and Scala 2.11).

@asfgit

This comment has been minimized.

Copy link

commented Jul 19, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/6153/
Test FAILed (JDK 8 and Scala 2.12).

@nicktrav nicktrav force-pushed the nicktrav:nickt.reassign-partitions-validation branch Jul 22, 2017
@nicktrav

This comment has been minimized.

Copy link
Contributor Author

commented Jul 22, 2017

@omkreddy - thanks for checking this one out! I completely forgot that I had this open. I've rebased.

Would love to get it merged, as it was the source of a reasonable amount of (avoidable) pain for us a few months ago, and would love to save some others from that. Are you able to merge this in when green?

@asfgit

This comment has been minimized.

Copy link

commented Jul 22, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.11/6281/
Test PASSed (JDK 7 and Scala 2.11).

@asfgit

This comment has been minimized.

Copy link

commented Jul 22, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/6265/
Test PASSed (JDK 8 and Scala 2.12).

@omkreddy

This comment has been minimized.

Copy link
Contributor

commented Jul 24, 2017

LGTM.

Though we are deprecating "--zookeeper" option in KIP-179 (Change ReassignPartitionsCommand to use adminClient), It is good to have this check.

cc @ijuma pinging for review.

@ijuma

This comment has been minimized.

Copy link
Contributor

commented Aug 4, 2017

Thanks for the PR. I think we should consider using Jackson to parse into classes with the expected structure. That will do the necessary validation in a more robust way. Thoughts?

@asfgit

This comment has been minimized.

Copy link

commented Aug 4, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.11/6565/
Test FAILed (JDK 7 and Scala 2.11).

@asfgit

This comment has been minimized.

Copy link

commented Aug 4, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/6550/
Test FAILed (JDK 8 and Scala 2.12).

@nicktrav

This comment has been minimized.

Copy link
Contributor Author

commented Aug 4, 2017

Sounds reasonable, @ijuma. I'll take another pass at it.

@nicktrav nicktrav force-pushed the nicktrav:nickt.reassign-partitions-validation branch Aug 5, 2017
@nicktrav

This comment has been minimized.

Copy link
Contributor Author

commented Aug 5, 2017

@ijuma - I've taken a different approach, as suggested, using case classes to hold the deserialized json. PTAL.

cc: @omkreddy

@asfgit

This comment has been minimized.

Copy link

commented Aug 5, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk7-scala2.11/6593/
Test PASSed (JDK 7 and Scala 2.11).

@asfgit

This comment has been minimized.

Copy link

commented Aug 5, 2017

Refer to this link for build results (access rights to CI server needed):
https://builds.apache.org/job/kafka-pr-jdk8-scala2.12/6578/
Test PASSed (JDK 8 and Scala 2.12).

@ijuma

This comment has been minimized.

Copy link
Contributor

commented Aug 8, 2017

Thanks, I'll take a look tomorrow.

@nicktrav

This comment has been minimized.

Copy link
Contributor Author

commented Aug 22, 2017

@ijuma - sorry to keep pinging you on this. How is this looking? Happy to address any feedback you might have.

@nicktrav

This comment has been minimized.

Copy link
Contributor Author

commented Sep 7, 2017

Ping @omkreddy

@omkreddy

This comment has been minimized.

Copy link
Contributor

commented Sep 8, 2017

@nicktrav Thanks for your patience. committers are busy with 0.11.0.1 release and other tasks. I am sure this will get merged into 1.0.0/October release. Pls, rebase the PR when you get a chance.

cc @ijuma

@nicktrav nicktrav force-pushed the nicktrav:nickt.reassign-partitions-validation branch Sep 17, 2017
@nicktrav

This comment has been minimized.

Copy link
Contributor Author

commented Sep 18, 2017

Retest this please.

@omkreddy

This comment has been minimized.

Copy link
Contributor

commented Jan 8, 2018

Can we use zkUtils.parsePartitionReassignmentData here
https://github.com/apache/kafka/blob/trunk/core/src/main/scala/kafka/admin/ReassignPartitionsCommand.scala#L245
above change will solve KAFKA-6084. This can be done in another PR.

@ijuma

This comment has been minimized.

Copy link
Contributor

commented Jan 8, 2018

@omkreddy Ideally, we should not be using ZkUtils in ReassignPartitionsCommand (i.e. we should use KafkaZkClient). Also, ReassignPartitionsCommand also parses a log_dirs field which is not handled in ZkUtils (or KafkaZkClient). Let's do that clean-up in a separate PR.

@nicktrav

This comment has been minimized.

Copy link
Contributor Author

commented Jan 11, 2018

@omkreddy @ijuma - any action for me to take here based on the last two comments?

@omkreddy

This comment has been minimized.

Copy link
Contributor

commented Jan 11, 2018

LGTM. +1

@ijuma

This comment has been minimized.

Copy link
Contributor

commented Jan 18, 2018

Sorry, I have just moved countries and won't have time until next week.

@nicktrav

This comment has been minimized.

Copy link
Contributor Author

commented Jan 18, 2018

No worries at all. Good luck with the move!

@nicktrav nicktrav force-pushed the nicktrav:nickt.reassign-partitions-validation branch Feb 8, 2018
@nicktrav

This comment has been minimized.

Copy link
Contributor Author

commented Mar 20, 2018

Any chance of an update on this @ijuma @omkreddy? (coincidentally, today is the 1 year birthday of the ticket! 🍰)

nicktrav and others added 11 commits Aug 5, 2017
…rsisting state in ZooKeeper

Prior to this, there have been instances where invalid data was allowed to be persisted in ZooKeeper, which causes ClassCastExceptions when a broker is restarted and reads this type-unsafe data.

Adds basic structural and type validation for the reassignment JSON via
introduction of Scala case classes that map to the expected JSON
structure.
@nicktrav nicktrav force-pushed the nicktrav:nickt.reassign-partitions-validation branch to ec7f36d Mar 20, 2018
@omkreddy

This comment has been minimized.

Copy link
Contributor

commented Mar 20, 2018

maybe @hachikuji can help to merge this patch.

Copy link
Contributor

left a comment

Although (as mentioned on my JIRA) this PR seems to aim bit different goals then mine, I reviewed this PR and made one minor comment/question.
All in all I think the PR looks good. Would be happy to see this merged.

* Parse a JSON string into either a generic type T, or a JsonProcessingException in the case of
* exception.
*/
def parseStringAs[T](input: String)(implicit tag: ClassTag[T]): Either[JsonProcessingException, T] = {

This comment has been minimized.

Copy link
@viktorsomogyi

viktorsomogyi Mar 20, 2018

Contributor

Hmm, just a very basic question: why are you using here Either/Left/Right instead of Try/Success/Failure?
As from my previous experiences we used Try when we wanted to propagate the exception.
And please not I don't mind using Either, just asking for reasons. :)

This comment has been minimized.

Copy link
@nicktrav

nicktrav Mar 22, 2018

Author Contributor

My (idiomatic) Scala isn't great, but this came at the suggestion of @ijuma (above):
#2708 (comment)

This comment has been minimized.

Copy link
@ijuma

ijuma Mar 23, 2018

Contributor

Try doesn't let you specify an exception subclass and behaves a bit different if an exception is thrown during map, etc.

@ijuma
ijuma approved these changes Mar 31, 2018
Copy link
Contributor

left a comment

I pushed minor changes and it now LGTM. Will merge it after the tests pass. Thanks so much for your patience.

@ijuma ijuma merged commit 4106cb1 into apache:trunk Mar 31, 2018
3 checks passed
3 checks passed
JDK 7 and Scala 2.11 SUCCESS 8760 tests run, 7 skipped, 0 failed.
Details
JDK 8 and Scala 2.12 SUCCESS 8760 tests run, 7 skipped, 0 failed.
Details
JDK 9 and Scala 2.12 SUCCESS 8760 tests run, 7 skipped, 0 failed.
Details
@nicktrav

This comment has been minimized.

Copy link
Contributor Author

commented Mar 31, 2018

Thanks @ijuma!

nimosunbit added a commit to sunbit-dev/kafka that referenced this pull request Nov 6, 2018
…sisting state in ZooKeeper (apache#2708)

Prior to this, there have been instances where invalid data was allowed to be persisted in
ZooKeeper, which causes ClassCastExceptions when a broker is restarted and reads this
type-unsafe data.

Adds basic structural and type validation for the reassignment JSON via
introduction of Scala case classes that map to the expected JSON
structure. Also use the Scala case classes to deserialize the JSON
to avoid duplication.

Reviewers: Manikumar Reddy <manikumar.reddy@gmail.com>, Viktor Somogyi <viktor.somogyi@cloudera.com>, Ismael Juma <ismael@juma.me.uk>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
6 participants
You can’t perform that action at this time.