Skip to content

KAFKA-5876: IQ should throw different exceptions for different errors(part 1)#8200

Merged
mjsax merged 3 commits into
apache:trunkfrom
vitojeng:kip-216
Jul 23, 2020
Merged

KAFKA-5876: IQ should throw different exceptions for different errors(part 1)#8200
mjsax merged 3 commits into
apache:trunkfrom
vitojeng:kip-216

Conversation

@vitojeng

@vitojeng vitojeng commented Mar 2, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor

KIP-216: IQ should throw different exceptions for different errors

KAFKA-5876's PR break into multiple parts. This PR is part 1: the new exceptions introduced in KIP-216

Committer Checklist (excluded from commit message)

  • Verify design and implementation
  • Verify test coverage and CI build status
  • Verify documentation (including upgrade notes)

@vvcephei

vvcephei commented Mar 2, 2020

Copy link
Copy Markdown
Contributor

test this please

@mjsax

mjsax commented Mar 5, 2020

Copy link
Copy Markdown
Member

Thanks for the PR @vitojeng -- I put it into my review backlog :)

@mjsax

mjsax commented Mar 7, 2020

Copy link
Copy Markdown
Member

@vitojeng This is a quite big PR -- would it make sense to break it into multiple? Maybe one that only add the new exception classed, and one each for using a new exception type?

@vitojeng

vitojeng commented Mar 7, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

@vitojeng This is a quite big PR -- would it make sense to break it into multiple? Maybe one that only add the new exception classed, and one each for using a new exception type?

@mjsax The same feeling. Break it into multiple part is a good way. I will force-push the new exceptions only in this PR first.

@vitojeng vitojeng changed the title KAFKA-5876: IQ should throw different exceptions for different errors KAFKA-5876: IQ should throw different exceptions for different errors(part 1) Mar 8, 2020
@mjsax

mjsax commented May 29, 2020

Copy link
Copy Markdown
Member

Hey @vitojeng -- sorry for the long delay... We had a big KIP review back log... I would dedicate review time now to get KIP-216 merged. Are you still interested in pushing it over the finish line? If yes, I would start with this PR in the next days.

@vitojeng

vitojeng commented May 29, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Hey @vitojeng -- sorry for the long delay... We had a big KIP review back log...

@mjsax Never mind, I understand. :)
I'll be rebase master again and update this PR ASAP.

@vitojeng

vitojeng commented May 29, 2020

Copy link
Copy Markdown
Contributor Author

@mjsax Just rebase & force-pushed.
You can start review when you available.

@mjsax

mjsax commented May 29, 2020

Copy link
Copy Markdown
Member

Retest this please.

1 similar comment
@mjsax

mjsax commented May 29, 2020

Copy link
Copy Markdown
Member

Retest this please.

@mjsax

mjsax commented May 29, 2020

Copy link
Copy Markdown
Member

Jenkins does not cooperator right now. Will try again later.

brary
brary reviewed Jun 10, 2020
View reviewed changes
Comment thread streams/src/main/java/org/apache/kafka/streams/errors/InvalidStateStoreException.java Outdated
Comment thread streams/src/main/java/org/apache/kafka/streams/errors/InvalidStateStorePartitionException.java Outdated
Comment thread streams/src/main/java/org/apache/kafka/streams/errors/StateStoreMigratedException.java Outdated
Comment thread streams/src/main/java/org/apache/kafka/streams/errors/StateStoreNotAvailableException.java Outdated
Comment thread streams/src/main/java/org/apache/kafka/streams/errors/UnknownStateStoreException.java Outdated
@vitojeng

vitojeng commented Jun 11, 2020

Copy link
Copy Markdown
Contributor Author

Address comments and rebase trunk.

@vitojeng

vitojeng commented Jun 11, 2020

Copy link
Copy Markdown
Contributor Author

Retest this please.

@mjsax

mjsax commented Jun 11, 2020

Copy link
Copy Markdown
Member

@vitojeng -- Jenkins got locked down a couple of week ago. Only committer can trigger builds now.

@vitojeng

vitojeng commented Jun 11, 2020

Copy link
Copy Markdown
Contributor Author

Got it!

@brary

brary commented Jun 11, 2020

Copy link
Copy Markdown
Contributor

LGTM!

@mjsax mjsax added the kip Requires or implements a KIP label Jun 12, 2020
mjsax
mjsax reviewed Jul 17, 2020
View reviewed changes

@mjsax mjsax left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry (again... :() for the delayed review. Couple of minor suggestions to improve the JavaDocs. The goal of the KIP was really to make it easier for users to reason about the type of error and thus we should be a little bit more elaborative.

Comment thread streams/src/main/java/org/apache/kafka/streams/errors/InvalidStateStoreException.java Outdated
Comment thread streams/src/main/java/org/apache/kafka/streams/errors/InvalidStateStorePartitionException.java Outdated
Comment thread streams/src/main/java/org/apache/kafka/streams/errors/StateStoreMigratedException.java Outdated
Comment thread streams/src/main/java/org/apache/kafka/streams/errors/StreamsNotStartedException.java Outdated
Comment thread streams/src/main/java/org/apache/kafka/streams/errors/StateStoreNotAvailableException.java Outdated
Comment thread streams/src/main/java/org/apache/kafka/streams/errors/StreamsNotStartedException.java Outdated
Comment thread streams/src/main/java/org/apache/kafka/streams/errors/StreamsRebalancingException.java Outdated
Comment thread streams/src/main/java/org/apache/kafka/streams/errors/UnknownStateStoreException.java Outdated
vitojeng added 2 commits July 18, 2020 23:17
KAFKA-5876: IQ should throw different exceptions for different errors…
12ae090 
…(part 1)

Add new sub-classes of InvalidStateStoreException
address comments
f3136d4
@vitojeng

vitojeng commented Jul 18, 2020

Copy link
Copy Markdown
Contributor Author

@mjsax Thanks for the review. Already address your comment. :)

mjsax
mjsax reviewed Jul 20, 2020
View reviewed changes

@mjsax mjsax left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the update @vitojeng -- a few more nits. Overall LGTM. Will merge after addressed.

Comment thread streams/src/main/java/org/apache/kafka/streams/errors/InvalidStateStoreException.java Outdated
Comment thread streams/src/main/java/org/apache/kafka/streams/errors/InvalidStateStoreException.java Outdated
Comment thread streams/src/main/java/org/apache/kafka/streams/errors/StreamsNotStartedException.java Outdated
@vitojeng

vitojeng commented Jul 21, 2020 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Thanks for the update @vitojeng -- a few more nits. Overall LGTM. Will merge after addressed.

Thanks @mjsax for the reviewing, just updated.

@mjsax

mjsax commented Jul 22, 2020

Copy link
Copy Markdown
Member

Retest this please.

mjsax
mjsax approved these changes Jul 22, 2020
View reviewed changes

@mjsax mjsax left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

Will merge after Jenkins passed.

@mjsax mjsax merged commit 2d79171 into apache:trunk Jul 23, 2020
@vitojeng vitojeng deleted the kip-216 branch July 23, 2020 02:55
@mjsax

mjsax commented Jul 23, 2020

Copy link
Copy Markdown
Member

Merged to trunk. Thanks for the PR and patience @vitojeng!

Looking forward to the next PR(s) -- it's best to continue doing multiple smaller PRs (I leave it up to your judgment what the best way is to split the work in chunks.)

@vitojeng

vitojeng commented Jul 23, 2020

Copy link
Copy Markdown
Contributor Author

Looking forward to the next PR(s) -- it's best to continue doing multiple smaller PRs (I leave it up to your judgment what the best way is to split the work in chunks.)

Thanks @mjsax !
I would separate each new exception into different PRs. I guess this should be a better way for reviewing. Also thanks @brary for the reviewing.

@vitojeng vitojeng mentioned this pull request Jul 24, 2020
Closed
3 tasks
ijuma added a commit to ijuma/kafka that referenced this pull request Nov 17, 2020
@ijuma
Merge remote-tracking branch 'apache-github/trunk' into skip-to-struc…
c430055 
…t-for-generated-requests

* apache-github/trunk: (148 commits)
  MINOR: remove NewTopic#NO_PARTITIONS and NewTopic#NO_REPLICATION_FACTOR as they are duplicate to CreateTopicsRequest#NO_NUM_PARTITIONS and CreateTopicsRequest#NO_REPLICATION_FACTOR (apache#9077)
  MINOR: Remove staticmethod tag to be able to use logger of instance (apache#9086)
  MINOR: Adjust 'release.py' script to use shell when using gradlewAll and PGP signing, which were required to build the 2.6.0 RCs (apache#9045)
  MINOR: Update dependencies for Kafka 2.7 (part 1) (apache#9082)
  MINOR: INFO log4j when request re-join (apache#9068)
  MINOR: Recommend Java 11 (apache#9080)
  KAFKA-10306: GlobalThread should fail on InvalidOffsetException (apache#9075)
  KAFKA-10158: Fix flaky testDescribeUnderReplicatedPartitionsWhenReassignmentIsInProgress (apache#9022)
  MINOR: code cleanup for `VOut` inconsistent naming (apache#8907)
  KAFKA-10246 : AbstractProcessorContext topic() throws NPE (apache#9034)
  KAFKA-10305: Print usage when parsing fails for ConsumerPerformance (apache#9071)
  MINOR: removed incorrect deprecation annotations (apache#9061)
  MINOR: speed up release script (apache#9070)
  MINOR: add task ':streams:testAll' (apache#9073)
  KAFKA-10301: Do not clear Partition#remoteReplicasMap during partition assignment updates (apache#9065)
  KAFKA-10268: dynamic config like "--delete-config log.retention.ms" doesn't work (apache#9051)
  KAFKA-10300 fix flaky core/group_mode_transactions_test.py (apache#9059)
  MINOR: Publish metrics package in the javadoc (apache#9036)
  KAFKA-8264: decrease the record size for flaky test
  KAFKA-5876: Add new exception types for Interactive Queries (apache#8200)
  ...
@vitojeng vitojeng mentioned this pull request Jan 4, 2021
Merged
3 tasks
@vitojeng vitojeng mentioned this pull request Apr 26, 2021
Merged
3 tasks
@vitojeng vitojeng mentioned this pull request May 9, 2021
Merged
3 tasks
@vitojeng vitojeng mentioned this pull request Jun 5, 2021
Closed
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@mjsax mjsax mjsax approved these changes

+1 more reviewer

@brary brary brary left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

kip Requires or implements a KIP streams

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@vitojeng @vvcephei @mjsax @brary