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

MINOR: Further reduce runtime for metrics integration tests #8514

Conversation

guozhangwang
Copy link
Contributor

  1. In both RocksDBMetrics and Metrics integration tests, we do not need to wait for consumer to consume records from output topics since the sensors / metrics are registered upon task creation.

  2. Merged the two test cases of RocksDB with one app that creates two state stores (non-segmented and segmented).

With these two changes, local runtime of these two tests reduced from 2min+ and 3min+ to under a minute.

Committer Checklist (excluded from commit message)

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

@guozhangwang
Copy link
Contributor Author

@cadonna @mjsax for reviews.

@mjsax mjsax added streams tests Test fixes (including flaky tests) labels Apr 19, 2020
@mjsax
Copy link
Member

mjsax commented Apr 19, 2020

Checkstyle error:

Task :streams:checkstyleTest
17:30:54 [ant:checkstyle] [ERROR] /home/jenkins/jenkins-slave/workspace/kafka-pr-jdk11-scala2.13/streams/src/test/java/org/apache/kafka/streams/integration/MetricsIntegrationTest.java:22:8: Unused import - org.apache.kafka.common.serialization.LongDeserializer. [UnusedImports]
17:30:54 [ant:checkstyle] [ERROR] /home/jenkins/jenkins-slave/workspace/kafka-pr-jdk11-scala2.13/streams/src/test/java/org/apache/kafka/streams/state/internals/RocksDBStoreTest.java:34:8: Unused import - org.apache.kafka.streams.processor.ProcessorContext. [UnusedImports]
17:30:54 [ant:checkstyle] [ERROR] /home/jenkins/jenkins-slave/workspace/kafka-pr-jdk11-scala2.13/streams/src/test/java/org/apache/kafka/streams/state/internals/RocksDBStoreTest.java:163:21: '=' is not preceded with whitespace. [WhitespaceAround]

new Properties()
),
STREAM_INPUT_TWO,
Collections.singleton(new KeyValue<>(1, "A")),
Copy link
Member

Choose a reason for hiding this comment

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

Why do we write into STREAM_INPUT_TWO with 3 calls instead of just one call passing in all 3 records at once?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Note we use a mocktime with auto tick WINDOW_SIZE.toMillis(): and each time its milliseconds() is called it would auto-advance, we need the produced records with those advanced timestamps.

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

Overall LGTM. Feel free to merge after checkstyle is fixed.

() -> "RocksDB metric bytes.written.total did not increase in " + TIMEOUT + " ms"
);
}

Copy link
Contributor

@cadonna cadonna Apr 20, 2020

Choose a reason for hiding this comment

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

Thank you! I forgot to delete this in my refactoring.

Copy link
Contributor

@cadonna cadonna left a comment

Choose a reason for hiding this comment

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

@guozhangwang LGTM
Truly awesome!

@guozhangwang guozhangwang merged commit fcf45e1 into apache:trunk Apr 20, 2020
@guozhangwang
Copy link
Contributor Author

Merged to trunk.

@guozhangwang guozhangwang deleted the KMinor-reduce-metrics-integration-runtime branch April 20, 2020 18:01
ijuma added a commit to confluentinc/kafka that referenced this pull request Apr 21, 2020
* apache-github/trunk:
  MINOR: Fix grammar in error message for InvalidRecordException (apache#8465)
  KAFKA-9868: Reduce number of transaction log partitions for embed broker (apache#8522)
  MINOR: Adding github whitelist (apache#8523)
  MINOR: Upgrade gradle plugins and test libraries for Java 14 support (apache#8519)
  MINOR: Further reduce runtime for metrics integration tests (apache#8514)
  MINOR: Use .asf.yaml to direct github notifications to JIRA and mailing lists (apache#8521)
  MINOR: Update to Gradle 6.3 (apache#7677)
  HOTFIX: fix checkstyle error of RocksDBStoreTest and flaky RocksDBTimestampedStoreTest.shouldOpenExistingStoreInRegularMode (apache#8515)
  MINOR: cleanup RocksDBStore tests  (apache#8510)
  KAFKA-9818: Fix flaky test in RecordCollectorTest (apache#8507)
  MINOR: reduce impact of trace logging in replica hot path (apache#8468)
  KAFKA-6145: KIP-441: Add test scenarios to ensure rebalance convergence (apache#8475)
  KAFKA-9881: Convert integration test to verify measurements from RocksDB to unit test (apache#8501)
  MINOR: improve test coverage for dynamic LogConfig(s) (apache#7616)
  MINOR: Switch order of sections on tumbling and hopping windows in streams doc. Tumbling windows are defined as "special case of hopping time windows" - but hopping windows currently only explained later in the docs. (apache#8505)
  KAFKA-9819: Fix flaky test in StoreChangelogReaderTest (apache#8488)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
streams tests Test fixes (including flaky tests)
Projects
None yet
3 participants