Skip to content

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

Merged
mjsax merged 3 commits intoapache: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 intoapache:trunkfrom
vitojeng:kip-216

Conversation

@vitojeng
Copy link
Copy Markdown
Contributor

@vitojeng vitojeng commented Mar 2, 2020

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
Copy link
Copy Markdown
Contributor

vvcephei commented Mar 2, 2020

test this please

@mjsax
Copy link
Copy Markdown
Member

mjsax commented Mar 5, 2020

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

@mjsax
Copy link
Copy Markdown
Member

mjsax commented Mar 7, 2020

@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
Copy link
Copy Markdown
Contributor Author

vitojeng commented Mar 7, 2020

@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
Copy link
Copy Markdown
Member

mjsax commented May 29, 2020

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
Copy link
Copy Markdown
Contributor Author

vitojeng commented May 29, 2020

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
Copy link
Copy Markdown
Contributor Author

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

@mjsax
Copy link
Copy Markdown
Member

mjsax commented May 29, 2020

Retest this please.

1 similar comment
@mjsax
Copy link
Copy Markdown
Member

mjsax commented May 29, 2020

Retest this please.

@mjsax
Copy link
Copy Markdown
Member

mjsax commented May 29, 2020

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

@vitojeng
Copy link
Copy Markdown
Contributor Author

Address comments and rebase trunk.

@vitojeng
Copy link
Copy Markdown
Contributor Author

Retest this please.

@mjsax
Copy link
Copy Markdown
Member

mjsax commented Jun 11, 2020

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

@vitojeng
Copy link
Copy Markdown
Contributor Author

Got it!

@brary
Copy link
Copy Markdown
Contributor

brary commented Jun 11, 2020

LGTM!

@mjsax mjsax added the kip Requires or implements a KIP label Jun 12, 2020
Copy link
Copy Markdown
Member

@mjsax mjsax left a comment

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.

vitojeng added 2 commits July 18, 2020 23:17
…(part 1)

Add new sub-classes of InvalidStateStoreException
@vitojeng
Copy link
Copy Markdown
Contributor Author

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

Copy link
Copy Markdown
Member

@mjsax mjsax left a comment

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.

@vitojeng
Copy link
Copy Markdown
Contributor Author

vitojeng commented Jul 21, 2020

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

Thanks @mjsax for the reviewing, just updated.

@mjsax
Copy link
Copy Markdown
Member

mjsax commented Jul 22, 2020

Retest this please.

Copy link
Copy Markdown
Member

@mjsax mjsax left a comment

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
Copy link
Copy Markdown
Member

mjsax commented Jul 23, 2020

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
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.

ijuma added a commit to ijuma/kafka that referenced this pull request Nov 17, 2020
…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)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kip Requires or implements a KIP streams

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants