-
Notifications
You must be signed in to change notification settings - Fork 3.7k
Fix flaky MessageIdTest and introduce some testing improvements #9286
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
Conversation
eolivelli
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
great work !
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerFactoryImpl.java
Outdated
Show resolved
Hide resolved
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerFactoryImpl.java
Outdated
Show resolved
Hide resolved
|
@codelipenghui @zymap @sijie @rdhabalia this patch is going to fix the most Flaky test in the suite. please take a look thank you @lhotari for contributing this work |
44ccd34 to
06f8229
Compare
merlimat
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very nice work, just a couple of comments on executors
pulsar-metadata/src/main/java/org/apache/pulsar/metadata/impl/AbstractMetadataStore.java
Outdated
Show resolved
Hide resolved
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerFactoryImpl.java
Outdated
Show resolved
Hide resolved
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerFactoryImpl.java
Outdated
Show resolved
Hide resolved
managed-ledger/src/main/java/org/apache/bookkeeper/mledger/impl/ManagedLedgerFactoryImpl.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/main/java/org/apache/pulsar/broker/service/BrokerService.java
Outdated
Show resolved
Hide resolved
pulsar-broker/src/test/java/org/apache/pulsar/client/impl/MessageChecksumTest.java
Show resolved
Hide resolved
pulsar-broker/src/test/java/org/apache/pulsar/client/impl/PulsarTestClient.java
Show resolved
Hide resolved
zymap
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice work!
- it makes it easier to override for tests
- useful for tests. Instead of relying on Mockito, there's a pure Java way to inject behavior to producer implementations for testing purposes
…ons and test flakiness - provides features for simulating failure conditions, for example the case of the broker connection disconnecting
06f8229 to
59d29f0
Compare
|
@lhotari It seems there's a genuine test failure: |
@merlimat Thanks for the heads up. I'll address it tomorrow. |
eolivelli
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
/pulsarbot run-failure-checks |
…he#9286) * Refactor PulsarClient initialization and lifecycle management in tests * Add getter and setter to access remoteEndpointProtocolVersion field - it makes it easier to override for tests * Add hooks for overriding the producer implementation in PulsarClientImpl - useful for tests. Instead of relying on Mockito, there's a pure Java way to inject behavior to producer implementations for testing purposes * Introduce PulsarTestClient that contains ways to prevent race conditions and test flakiness - provides features for simulating failure conditions, for example the case of the broker connection disconnecting * Add solution for using Enums classes as source for TestNG DataProvider * Fix flaky MessageIdTest and move checksum related tests to new class * Fix NPE in PartitionedProducerImplTest
Motivation
MessageIdTest class contains 2 of the flaky test cases in the Pulsar code base:
org.apache.pulsar.client.impl.MessageIdTest.testChecksumVersionComptabilityandorg.apache.pulsar.client.impl.MessageIdTest.testChecksumReconnectionThese test cases don't have much to do with
MessageId, but are tests for validating message checksum handling in cases where there are pre 1.15 version brokers and post 1.15 version brokers in a mixed broker environment. The tests might not be very relevant any more. However it was taken as a learning experiment to fix these tests and refactor them so that the flakiness of the test code would be eliminated. Similar patterns might be needed in other tests to eliminate flakiness.Modifications
The changes aren't only to fix MessageIdTest. Most changes could help reduce flakiness of other tests as well.
Improve shutdown of the broker and related services to reduce test flakiness
so that in-flight tasks get processed
Handle special case where the executor rejects the task and the callback was never called
Improve logging in MockedPulsarServiceBaseTest related to stopping and starting
Refactor PulsarClient initialization and lifecycle management in tests
Add getter and setter to access remoteEndpointProtocolVersion field
Add hooks for overriding the producer implementation in PulsarClientImpl
way to inject behavior to producer implementations for testing purposes
Introduce PulsarTestClient that contains ways to prevent race conditions and test flakiness
the case of the broker connection disconnecting
Add solution for using Enums classes as source for TestNG DataProvider
Fix flaky MessageIdTest and move checksum related tests to new class