MINOR: Ignore test_broker_type_bounce_at_start system test #5055
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ijuma
reviewed
May 21, 2018
| @@ -171,6 +172,9 @@ public void makeReady(final Map<String, InternalTopicConfig> topics) { | |||
| */ | |||
| // visible for testing | |||
| protected Map<String, Integer> getNumPartitions(final Set<String> topics) { | |||
| log.debug("Trying to check if topics {} have been created with expected number of partitions.", topics); | |||
|
|
|||
| // TODO: KAFKA-6928. should not need retries in the outer caller as it will be retried internally in admin client | |||
There was a problem hiding this comment.
Let's avoid TODOs in the code. We can point to the relevant part of the code in the JIRA.
ijuma
reviewed
May 21, 2018
| @@ -577,7 +577,9 @@ final void fail(long now, Throwable throwable) { | |||
| // this RPC. That is why 'tries' is not incremented. | |||
| if ((throwable instanceof UnsupportedVersionException) && | |||
| handleUnsupportedVersionException((UnsupportedVersionException) throwable)) { | |||
| log.trace("{} attempting protocol downgrade.", this); | |||
| if (log.isDebugEnabled()) { | |||
There was a problem hiding this comment.
There's no need for this conditional, right?
There was a problem hiding this comment.
A bit unfortunate that this comment was ignored, it caused a checkstyle error with the new checkstyle:
mjsax
approved these changes
May 21, 2018
ying-zheng
pushed a commit
to ying-zheng/kafka
that referenced
this pull request
Jul 6, 2018
test_broker_type_bounce_at_start tries to validate that when the controller is down, the streams client will always fail trying to create the topic; with the current behavior of admin client it is actually not always true: the actual behavior depends on the admin client internals as well as when the controller becomes unavailable during the leader assign partitions phase. I'd suggest at least ignore this test for now until the admin client has more stable (personally I'd even suggest removing this test as its coverage benefits is smaller than its introduced issues to me). Also adding a few more log4j entries as a result of investigating this issue. Reviewers: Matthias J. Sax <[email protected]>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
test_broker_type_bounce_at_start tries to validate that when the controller is down, the streams client will always fail trying to create the topic; with the current behavior of admin client it is actually not always true: the actual behavior depends on the admin client internals as well as when the controller becomes unavailable during the leader assign partitions phase. I'd suggest at least ignore this test for now until the admin client has more stable (personally I'd even suggest removing this test as its coverage benefits is smaller than its introduced issues to me).
Also adding a few more log4j entries as a result of investigating this issue.
Committer Checklist (excluded from commit message)