KAFKA-8262, KAFKA-8263: Fix flaky test MetricsIntegrationTest#6922
Merged
guozhangwang merged 1 commit intoapache:trunkfrom Jun 12, 2019
Merged
Conversation
- Timeout occurred due to initial slow rebalancing. - Added code to wait until `KafkaStreams` instance is in state RUNNING to check registration of metrics and in state NOT_RUNNING to check deregistration of metrics. - I removed all other wait conditions, because they are not needed if `KafkaStreams` instance is in the right state.
Member
Author
|
Call for quick review: @mjsax @abbccdda @ableegoldman @vvcephei @bbejeck @guozhangwang |
Contributor
guozhangwang
left a comment
guozhangwang
left a comment
There was a problem hiding this comment.
The refactoring lgtm. I just have one question if there are still any lazily registered metrics left, and if yes it's probably not safe to always assume all metrics are there after transit to RUNNING.
| return listMetricAfterClosingApp.size() == 0; | ||
| }, 10000, "de-registration of metrics"); | ||
| private void checkMetricDeregistration() { | ||
| final List<Metric> listMetricAfterClosingApp = new ArrayList<Metric>(kafkaStreams.metrics().values()).stream() |
Contributor
There was a problem hiding this comment.
I remember some of the metrics were lazily registered, i.e. they would only be registered if the corresponding action is called for the first time. Have we refactored it to always register all metrics up starting the task / process-node etc? Otherwise waiting for the stream state to transit to RUNNING may not guarantee all metrics should be already registered.
Member
Author
There was a problem hiding this comment.
Good to know! Will check that.
Member
Author
There was a problem hiding this comment.
@guozhangwang, I think it is fine just to wait until the Kafka Streams client is in state RUNNING. The test -- now and before the changes in this PR -- does not produce any records to the input topic. Hence, there is no other later event than the change to state RUNNING that could trigger a lazy metric registration. Does this make sense or am I missing something?
I also checked where the metrics are registered and they are registered either in constructors or init-methods. As far as I saw in the code, both types of methods are called during initialisation of the Kafka client, i.e., before the state change to RUNNING.
Contributor
There was a problem hiding this comment.
Thanks for double checking this! Then it lgtm.
3 tasks
Contributor
|
Merged to trunk. |
pengxiaolong
pushed a commit
to pengxiaolong/kafka
that referenced
this pull request
Jun 14, 2019
…he#6922) - Timeout occurred due to initial slow rebalancing. - Added code to wait until `KafkaStreams` instance is in state RUNNING to check registration of metrics and in state NOT_RUNNING to check deregistration of metrics. - I removed all other wait conditions, because they are not needed if `KafkaStreams` instance is in the right state. Reviewers: Guozhang Wang <wangguoz@gmail.com>
cadonna
added a commit
to cadonna/kafka
that referenced
this pull request
Oct 7, 2019
…he#6922) - Timeout occurred due to initial slow rebalancing. - Added code to wait until `KafkaStreams` instance is in state RUNNING to check registration of metrics and in state NOT_RUNNING to check deregistration of metrics. - I removed all other wait conditions, because they are not needed if `KafkaStreams` instance is in the right state. Reviewers: Guozhang Wang <wangguoz@gmail.com>
guozhangwang
pushed a commit
that referenced
this pull request
Oct 7, 2019
… (#7457) - Timeout occurred due to initial slow rebalancing. - Added code to wait until `KafkaStreams` instance is in state RUNNING to check registration of metrics and in state NOT_RUNNING to check deregistration of metrics. - I removed all other wait conditions, because they are not needed if `KafkaStreams` instance is in the right state. Reviewers: Guozhang Wang <wangguoz@gmail.com>
sdandu-gh
added a commit
to confluentinc/kafka
that referenced
this pull request
Nov 9, 2019
* KAFKA-8649: send latest commonly supported version in assignment (apache#7425) Reviewers: Matthias J. Sax <matthias@confluent.io> * KAFKA-8262, KAFKA-8263: Fix flaky test `MetricsIntegrationTest` (apache#6922) (apache#7457) - Timeout occurred due to initial slow rebalancing. - Added code to wait until `KafkaStreams` instance is in state RUNNING to check registration of metrics and in state NOT_RUNNING to check deregistration of metrics. - I removed all other wait conditions, because they are not needed if `KafkaStreams` instance is in the right state. Reviewers: Guozhang Wang <wangguoz@gmail.com> * MINOR: clarify wording around fault-tolerant state stores (apache#7510) Reviewers: Matthias J. Sax <matthias@confluent.io>, Guozhang Wang <guozhang@confluent.io> * MINOR: Port changes from trunk for test stability to 2.3 branch (apache#7424) Reviewer: Matthias J. Sax <matthias@confluent.io> * KAFKA-9014: Fix AssertionError when SourceTask.poll returns an empty list (apache#7491) Author: Konstantine Karantasis <konstantine@confluent.io> Reviewer: Randall Hauch <rhauch@gmail.com> * KAFKA-8945/KAFKA-8947: Fix bugs in Connect REST extension API (apache#7392) Fix bug in Connect REST extension API caused by invalid constructor parameter validation, and update integration test to play nicely with Jenkins Fix instantiation of TaskState objects by Connect framework. Author: Chris Egerton <chrise@confluent.io> Reviewers: Magesh Nandakumar <mageshn@confluent.io>, Randall Hauch <rhauch@gmail.com> * KAFKA-8340, KAFKA-8819: Use PluginClassLoader while statically initializing plugins (apache#7315) Added plugin isolation unit tests for various scenarios, with a `TestPlugins` class that compiles and builds multiple test plugins without them being on the classpath and verifies that the Plugins and DelegatingClassLoader behave properly. These initially failed for several cases, but now pass since the issues have been fixed. KAFKA-8340 and KAFKA-8819 are closely related, and this fix corrects the problems reported in both issues. Author: Greg Harris <gregh@confluent.io> Reviewers: Chris Egerton <chrise@confluent.io>, Magesh Nandakumar <mageshn@confluent.io>, Konstantine Karantasis <konstantine@confluent.io>, Randall Hauch <rhauch@gmail.com> * MINOR: log reason for fatal error in locking state dir (apache#7534) Reviewers: Bill Bejeck <bill@confluent.io>, Matthias J. Sax <matthias@confluent.io> * KAKFA-8950: Fix KafkaConsumer Fetcher breaking on concurrent disconnect (apache#7511) The KafkaConsumer Fetcher can sometimes get into an invalid state where it believes that there are ongoing fetch requests, but in fact there are none. This may be caused by the heartbeat thread concurrently handling a disconnection event just after the fetcher thread submits a request which would cause the Fetcher to enter an invalid state where it believes it has ongoing requests to the disconnected node but in fact it does not. This is due to a thread safety issue in the Fetcher where it was possible for the ordering of the modifications to the nodesWithPendingFetchRequests to be incorrect - the Fetcher was adding it after the listener had already been invoked, which would mean that pending node never gets removed again. This PR addresses that thread safety issue by ensuring that the pending node is added to the nodesWithPendingFetchRequests before the listener is added to the future, ensuring the finally block is called after the node is added. Reviewers: Tom Lee, Jason Gustafson <jason@confluent.io>, Rajini Sivaram <rajinisivaram@googlemail.com> * Fix bug in AssignmentInfo#encode and add additional logging (apache#7545) Same as apache#7537 but targeted at 2.3 for cherry-pick Reviewers: Bill Bejeck <bbejeck@gmail.com> * Bump version to 2.3.1 * Update versions to 2.3.2-SNAPSHOT * HOTFIX: fix bug in VP test where it greps for the wrong log message (apache#7643) Reviewers: Guozhang Wang <wangguoz@gmail.com>
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
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.
KafkaStreamsinstance is in state RUNNING to check registration ofmetrics and in state NOT_RUNNING to check deregistration of metrics.
KafkaStreamsinstance is inthe right state.
Committer Checklist (excluded from commit message)