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

feat(e2e): move e2e tests #328

Merged
merged 3 commits into from
Sep 1, 2023
Merged

feat(e2e): move e2e tests #328

merged 3 commits into from
Sep 1, 2023

Conversation

jeqo
Copy link
Contributor

@jeqo jeqo commented Jul 21, 2023

See commits for details on changes.

Depends on

@jeqo jeqo force-pushed the e2e-tests branch 3 times, most recently from 6c75023 to c59042b Compare August 10, 2023 12:30
@jeqo jeqo marked this pull request as ready for review August 10, 2023 13:26
@jeqo jeqo force-pushed the e2e-tests branch 2 times, most recently from 8498791 to 2a18510 Compare August 15, 2023 06:46
@jeqo jeqo requested a review from a team as a code owner August 15, 2023 06:46
@jeqo jeqo added the blocked Has a dependent PR/issue that is not merged/solved yet label Aug 17, 2023
@jeqo jeqo removed the blocked Has a dependent PR/issue that is not merged/solved yet label Aug 18, 2023
@jeqo jeqo force-pushed the e2e-tests branch 6 times, most recently from d5220ef to a58c3e7 Compare August 21, 2023 18:41
@jeqo jeqo force-pushed the e2e-tests branch 8 times, most recently from d956bc1 to c846b28 Compare August 22, 2023 07:12
@jeqo
Copy link
Contributor Author

jeqo commented Aug 23, 2023

we can probably continue using Java 17 for the e2e tests? (You only need the Gradle to run under Java 17 itself, which is probably fine).

@ivanyu we could do this, but I'd prefer to keep the same JDK version across modules to avoid confusion. We can migrate back to records once JDK 11 is dropped at some point (🤞🏽), wdyt?

@jeqo jeqo added the blocked Has a dependent PR/issue that is not merged/solved yet label Aug 23, 2023
Comment on lines +521 to +529
@Test
@Disabled("https://github.com/aiven/kafka/issues/35")
@Order(5)
void topicDelete() throws Exception {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

topic delete with tiered storage is still flaky. Disabling it in the meantime, we can troubleshoot this after switching to upstream.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

     [2023-08-23 00:32:07,465] ERROR [RemoteLogManager=1 partition=4OkS_EQaSVKfGYZVj2w9bg:topic1-0] Error occurred while copying log segments of partition: 4OkS_EQaSVKfGYZVj2w9bg:topic1-0 (kafka.log.remote.RemoteLogManager$RLMTask)
    java.lang.IllegalStateException: This instance is in invalid state, initialized: false close: false
    	at org.apache.kafka.server.log.remote.metadata.storage.TopicBasedRemoteLogMetadataManager.ensureInitializedAndNotClosed(TopicBasedRemoteLogMetadataManager.java:501)
    	at org.apache.kafka.server.log.remote.metadata.storage.TopicBasedRemoteLogMetadataManager.highestOffsetForEpoch(TopicBasedRemoteLogMetadataManager.java:224)
    	at kafka.log.remote.RemoteLogManager.$anonfun$findHighestRemoteOffset$2(RemoteLogManager.scala:612)
    	at kafka.log.remote.RemoteLogManager.$anonfun$findHighestRemoteOffset$2$adapted(RemoteLogManager.scala:609)
    	at scala.Option.foreach(Option.scala:437)
    	at kafka.log.remote.RemoteLogManager.$anonfun$findHighestRemoteOffset$1(RemoteLogManager.scala:609)
    	at kafka.log.remote.RemoteLogManager.$anonfun$findHighestRemoteOffset$1$adapted(RemoteLogManager.scala:608)
    	at scala.Option.foreach(Option.scala:437)
    	at kafka.log.remote.RemoteLogManager.findHighestRemoteOffset(RemoteLogManager.scala:608)
    	at kafka.log.remote.RemoteLogManager$RLMTask.maybeUpdateReadOffset$1(RemoteLogManager.scala:394)
    	at kafka.log.remote.RemoteLogManager$RLMTask.copyLogSegmentsToRemote(RemoteLogManager.scala:399)
    	at kafka.log.remote.RemoteLogManager$RLMTask.run(RemoteLogManager.scala:576)
    	at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:515)
    	at java.base/java.util.concurrent.FutureTask.runAndReset(FutureTask.java:305)
    	at java.base/java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:305)
    	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
    	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
    	at java.base/java.lang.Thread.run(Thread.java:829)
    [2023-08-23 00:32:07,469] ERROR [RemoteLogManager=1 partition=4OkS_EQaSVKfGYZVj2w9bg:topic1-0] Error while cleaning up log segments for partition: 4OkS_EQaSVKfGYZVj2w9bg:topic1-0 (kafka.log.remote.RemoteLogManager$RLMTask)
    java.lang.IllegalStateException: This instance is in invalid state, initialized: false close: false
    	at org.apache.kafka.server.log.remote.metadata.storage.TopicBasedRemoteLogMetadataManager.ensureInitializedAndNotClosed(TopicBasedRemoteLogMetadataManager.java:501)
    	at org.apache.kafka.server.log.remote.metadata.storage.TopicBasedRemoteLogMetadataManager.listRemoteLogSegments(TopicBasedRemoteLogMetadataManager.java:240)
    	at kafka.log.remote.RemoteLogManager$RLMTask.handleExpiredRemoteLogSegments(RemoteLogManager.scala:505)
    	at kafka.log.remote.RemoteLogManager$RLMTask.run(RemoteLogManager.scala:580)
    	at java.base/java.util.concurrent.Executors$RunnableAdapter.call(Executors.java:515)
    	at java.base/java.util.concurrent.FutureTask.runAndReset(FutureTask.java:305)
    	at java.base/java.util.concurrent.ScheduledThreadPoolExecutor$ScheduledFutureTask.run(ScheduledThreadPoolExecutor.java:305)
    	at java.base/java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1128)
    	at java.base/java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:628)
    	at java.base/java.lang.Thread.run(Thread.java:829)
    [2023-08-23 00:32:07,467] INFO [Broker id=1] Finished LeaderAndIsr request in 303ms correlationId 1 from controller 1 for 1 partitions (state.change.logger)

@jeqo jeqo requested review from AnatolyPopov and ivanyu August 23, 2023 08:47
@jeqo jeqo removed the blocked Has a dependent PR/issue that is not merged/solved yet label Aug 23, 2023
@jeqo jeqo force-pushed the e2e-tests branch 2 times, most recently from e6036b8 to 6803f71 Compare August 23, 2023 10:08
@jeqo jeqo requested a review from AnatolyPopov August 23, 2023 19:49
@jeqo jeqo force-pushed the e2e-tests branch 3 times, most recently from 70bca21 to 6ac5c7d Compare August 23, 2023 22:14
Comment on lines +521 to +520
await()
.between(Duration.ofSeconds(1), Duration.ofSeconds(30))
.pollInterval(Duration.ofSeconds(1))
.until(() -> assertNoTopicDataOnTierStorage(t0p0.topic(), t0p0.topicId()));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This was the last bit that seemed to be flaky. Before it was only checked once and there was no guarantee that files were removed. Now, it's checked for a bit. Builds seem to be more stable now.

*
* <p>Needed to allow running {@code SingleBrokerTest#stopKafka} after all tests.
*/
public static class FailFastTestWatcher implements TestWatcher {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we instead implement ExecutionCondition and make failed a field of this class to avoid constant checking?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Looking at the ExecutionCondition lifecycle, I guess we need both: watcher to set the flag when a test fails, and condition to do not run a test if failed = true.

Copy link
Contributor

@AnatolyPopov AnatolyPopov Aug 29, 2023

Choose a reason for hiding this comment

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

Exactly, that's what I was trying to say but used the wrong wording sorry.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No worries, changes are applied in the latest commit. Have a look. Thanks!

@jeqo jeqo requested a review from AnatolyPopov August 29, 2023 10:14
Copy link
Contributor

@AnatolyPopov AnatolyPopov left a comment

Choose a reason for hiding this comment

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

LGTM

@AnatolyPopov AnatolyPopov merged commit af418cf into main Sep 1, 2023
@AnatolyPopov AnatolyPopov deleted the e2e-tests branch September 1, 2023 13:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants