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.

Sign up for GitHub

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

Jump to bottom

Revert "Revert "KAFKA-15661: KIP-951: Server side changes (#14444)" #14747

Merged
merged 1 commit into from
Nov 16, 2023
Merged

Revert "Revert "KAFKA-15661: KIP-951: Server side changes (#14444)" #14747

merged 1 commit into from
Nov 16, 2023

Conversation

chb2ab
Copy link
Contributor

@chb2ab chb2ab commented Nov 13, 2023 •
edited
Loading

This KIP-951 commit was reverted to investigate the org.apache.kafka.tiered.storage.integration.ReassignReplicaShrinkTest test failure (#14738).

A fix for that was merged in #14757, hence unreverting this change.

This reverts commit a98bd7d.

Committer Checklist (excluded from commit message)

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

@chb2ab chb2ab force-pushed the chb2ab-kip951-serverside-unrevert branch from 760ba31 to 82eb06b Compare November 15, 2023 13:31
@chb2ab
Copy link
Contributor Author

chb2ab commented Nov 15, 2023

I've run all the failing tests locally and they passed (kafka.api.ConsumerBounceTest.testConsumptionWithBrokerFailures after a retry)

Build / JDK 8 and Scala 2.12 / testRackAwareRangeAssignor(String).quorum=kraft – integration.kafka.server.FetchFromFollowerIntegrationTest
Build / JDK 17 and Scala 2.13 / testRackAwareRangeAssignor(String).quorum=zk – integration.kafka.server.FetchFromFollowerIntegrationTest
Build / JDK 17 and Scala 2.13 / testRackAwareRangeAssignor(String).quorum=kraft – integration.kafka.server.FetchFromFollowerIntegrationTest
Build / JDK 17 and Scala 2.13 / testConsumptionWithBrokerFailures() – kafka.api.ConsumerBounceTest
Build / JDK 21 and Scala 2.13 / testAutoCreateTopic(String).quorum=zk – kafka.api.PlaintextProducerSendTest
Build / JDK 21 and Scala 2.13 / testTransactionAfterTransactionIdExpiresButProducerIdRemains(String).quorum=kraft – kafka.api.ProducerIdExpirationTest
Build / JDK 17 and Scala 2.13 / testDynamicListenerConnectionCreationRateQuota() – kafka.network.DynamicConnectionQuotaTest
Build / JDK 11 and Scala 2.13 / [3] Type=ZK, MetadataVersion=3.6-IV2, Security=PLAINTEXT – kafka.zk.ZkMigrationIntegrationTest
Build / JDK 21 and Scala 2.13 / [2] tlsProtocol=TLSv1.2, useInlinePem=true – org.apache.kafka.common.network.SslTransportLayerTest
Build / JDK 11 and Scala 2.13 / testOffsetTranslationBehindReplicationFlow() – org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationExactlyOnceTest
Build / JDK 17 and Scala 2.13 / testFenceMultipleBrokers() – org.apache.kafka.controller.QuorumControllerTest
Build / JDK 17 and Scala 2.13 / testBalancePartitionLeaders() – org.apache.kafka.controller.QuorumControllerTest
Build / JDK 11 and Scala 2.13 / testNoPublishEmptyImage() – org.apache.kafka.image.loader.MetadataLoaderTest
Build / JDK 21 and Scala 2.13 / testRemoteLogSizeCalculationWithSegmentsOfDifferentEpochs() – org.apache.kafka.server.log.remote.metadata.storage.TopicBasedRemoteLogMetadataManagerTest
Build / JDK 8 and Scala 2.12 / shouldAddNamedTopologyToRunningApplicationWithSingleInitialNamedTopology() – org.apache.kafka.streams.integration.NamedTopologyIntegrationTest
Build / JDK 21 and Scala 2.13 / shouldAddAndRemoveNamedTopologiesBeforeStartingAndRouteQueriesToCorrectTopology() – org.apache.kafka.streams.integration.NamedTopologyIntegrationTest

msn-tldr
msn-tldr approved these changes Nov 15, 2023
View reviewed changes
Copy link
Contributor

@msn-tldr msn-tldr left a comment

Choose a reason for hiding this comment

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

+1

@jolshan
Copy link
Contributor

jolshan commented Nov 15, 2023

@chb2ab was the re-revert clean? Any conflicts that needed resolving?

@chb2ab
Copy link
Contributor Author

chb2ab commented Nov 15, 2023 •
edited
Loading

@jolshan yes it was a clean re-revert, no conflicts.

@chb2ab
Copy link
Contributor Author

chb2ab commented Nov 16, 2023

I've run all the failing tests locally and they passed (kafka.api.ConsumerBounceTest.testConsumptionWithBrokerFailures after a retry)

Build / JDK 8 and Scala 2.12 / testRackAwareRangeAssignor(String).quorum=kraft – integration.kafka.server.FetchFromFollowerIntegrationTest
Build / JDK 17 and Scala 2.13 / testRackAwareRangeAssignor(String).quorum=zk – integration.kafka.server.FetchFromFollowerIntegrationTest
Build / JDK 17 and Scala 2.13 / testRackAwareRangeAssignor(String).quorum=kraft – integration.kafka.server.FetchFromFollowerIntegrationTest
Build / JDK 17 and Scala 2.13 / testConsumptionWithBrokerFailures() – kafka.api.ConsumerBounceTest
Build / JDK 21 and Scala 2.13 / testAutoCreateTopic(String).quorum=zk – kafka.api.PlaintextProducerSendTest
Build / JDK 21 and Scala 2.13 / testTransactionAfterTransactionIdExpiresButProducerIdRemains(String).quorum=kraft – kafka.api.ProducerIdExpirationTest
Build / JDK 17 and Scala 2.13 / testDynamicListenerConnectionCreationRateQuota() – kafka.network.DynamicConnectionQuotaTest
Build / JDK 11 and Scala 2.13 / [3] Type=ZK, MetadataVersion=3.6-IV2, Security=PLAINTEXT – kafka.zk.ZkMigrationIntegrationTest
Build / JDK 21 and Scala 2.13 / [2] tlsProtocol=TLSv1.2, useInlinePem=true – org.apache.kafka.common.network.SslTransportLayerTest
Build / JDK 11 and Scala 2.13 / testOffsetTranslationBehindReplicationFlow() – org.apache.kafka.connect.mirror.integration.MirrorConnectorsIntegrationExactlyOnceTest
Build / JDK 17 and Scala 2.13 / testFenceMultipleBrokers() – org.apache.kafka.controller.QuorumControllerTest
Build / JDK 17 and Scala 2.13 / testBalancePartitionLeaders() – org.apache.kafka.controller.QuorumControllerTest
Build / JDK 11 and Scala 2.13 / testNoPublishEmptyImage() – org.apache.kafka.image.loader.MetadataLoaderTest
Build / JDK 21 and Scala 2.13 / testRemoteLogSizeCalculationWithSegmentsOfDifferentEpochs() – org.apache.kafka.server.log.remote.metadata.storage.TopicBasedRemoteLogMetadataManagerTest
Build / JDK 8 and Scala 2.12 / shouldAddNamedTopologyToRunningApplicationWithSingleInitialNamedTopology() – org.apache.kafka.streams.integration.NamedTopologyIntegrationTest
Build / JDK 21 and Scala 2.13 / shouldAddAndRemoveNamedTopologiesBeforeStartingAndRouteQueriesToCorrectTopology() – org.apache.kafka.streams.integration.NamedTopologyIntegrationTest

Given these tests pass locally, I guess it's likely a hardware issue causing them to fail in Jenkins? I'm running on a MacBook Pro (2019, Monterey) with a 2.6 GHz 6-Core Intel Core i7 processor, I'm not sure what Jenkins is using or how to change the instance type. cc @dajac @jolshan

@jolshan
Copy link
Contributor

jolshan commented Nov 16, 2023 •
edited
Loading

@chb2ab don't worry about changing instance type. AK builds tend to run slower than local machines.
Most of these are consistent with trunk. I think the only two that were questionable were:
kafka.api.ConsumerBounceTest.testConsumptionWithBrokerFailures kafka.api.PlaintextProducerSendTest.testAutoCreateTopic

You mentioned offline that running these several times locally for your branch and trunk yielded about the same results.
I also cross-checked a few runs from the reverted PR runs and didn't seem them there.

@jolshan jolshan merged commit b1d83e2 into apache:trunk Nov 16, 2023
1 check failed
rreddy-22 pushed a commit to rreddy-22/kafka-rreddy that referenced this pull request Jan 2, 2024
@chb2ab @rreddy-22
Revert "Revert "KAFKA-15661: KIP-951: Server side changes (apache#14444…
dccb13a 
…)" (apache#14738)" (apache#14747)

This KIP-951 commit was reverted to investigate the org.apache.kafka.tiered.storage.integration.ReassignReplicaShrinkTest test failure (apache#14738).

A fix for that was merged in apache#14757, hence unreverting this change.

This reverts commit a98bd7d.

Reviewers: Justine Olshan <jolshan@confluent.io>, Mayank Shekhar Narula <mayanks.narula@gmail.com>
yyu1993 pushed a commit to yyu1993/kafka that referenced this pull request Feb 15, 2024
@chb2ab @yyu1993
Revert "Revert "KAFKA-15661: KIP-951: Server side changes (apache#14444…
e389803 
…)" (apache#14738)" (apache#14747)

This KIP-951 commit was reverted to investigate the org.apache.kafka.tiered.storage.integration.ReassignReplicaShrinkTest test failure (apache#14738).

A fix for that was merged in apache#14757, hence unreverting this change.

This reverts commit a98bd7d.

Reviewers: Justine Olshan <jolshan@confluent.io>, Mayank Shekhar Narula <mayanks.narula@gmail.com>
AnatolyPopov pushed a commit to aiven/kafka that referenced this pull request Feb 16, 2024
@chb2ab @AnatolyPopov
Revert "Revert "KAFKA-15661: KIP-951: Server side changes (apache#14444…
292f676 
…)" (apache#14738)" (apache#14747)

This KIP-951 commit was reverted to investigate the org.apache.kafka.tiered.storage.integration.ReassignReplicaShrinkTest test failure (apache#14738).

A fix for that was merged in apache#14757, hence unreverting this change.

This reverts commit a98bd7d.

Reviewers: Justine Olshan <jolshan@confluent.io>, Mayank Shekhar Narula <mayanks.narula@gmail.com>
clolov pushed a commit to clolov/kafka that referenced this pull request Apr 5, 2024
@chb2ab @clolov
Revert "Revert "KAFKA-15661: KIP-951: Server side changes (apache#14444…
fb48976 
…)" (apache#14738)" (apache#14747)

This KIP-951 commit was reverted to investigate the org.apache.kafka.tiered.storage.integration.ReassignReplicaShrinkTest test failure (apache#14738).

A fix for that was merged in apache#14757, hence unreverting this change.

This reverts commit a98bd7d.

Reviewers: Justine Olshan <jolshan@confluent.io>, Mayank Shekhar Narula <mayanks.narula@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@msn-tldr msn-tldr msn-tldr approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@chb2ab @jolshan @msn-tldr