Skip to content

KAFKA-3522: Remove TimestampedByteStore from public API#6222

Merged
mjsax merged 1 commit intoapache:2.2from
mjsax:kafka-3522-rocksdb-format-make-public-api-private
Feb 11, 2019
Merged

KAFKA-3522: Remove TimestampedByteStore from public API#6222
mjsax merged 1 commit intoapache:2.2from
mjsax:kafka-3522-rocksdb-format-make-public-api-private

Conversation

@mjsax
Copy link
Copy Markdown
Member

@mjsax mjsax commented Feb 2, 2019

Because KIP-258 slips, this PR moves TimestampedByteStore into internal package to not leak public API changes in 2.2 release. We will move it back, after 2.2 branch it cut.

@mjsax mjsax added the streams label Feb 2, 2019
@mjsax
Copy link
Copy Markdown
Member Author

mjsax commented Feb 2, 2019

Call for review @guozhangwang @bbejeck @vvcephei @ableegoldman

@mjsax
Copy link
Copy Markdown
Member Author

mjsax commented Feb 2, 2019

Retest this please.

Copy link
Copy Markdown
Contributor

@vvcephei vvcephei left a comment

Choose a reason for hiding this comment

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

Huh! Interesting solution, rather than reverting it and having a much bigger diff to pull out and put back in.

I've searched also for other public APIs from the KIP, and I agree this looks like the only one that was in so far.

Nice work; looking forward to getting this in in 2.3!

@mjsax
Copy link
Copy Markdown
Member Author

mjsax commented Feb 3, 2019

https://builds.apache.org/job/kafka-pr-jdk11-scala2.12/2125/

java.lang.AssertionError: Condition not met within timeout 15000. Connector tasks were not assigned a partition each.
	at org.apache.kafka.test.TestUtils.waitForCondition(TestUtils.java:365)
	at org.apache.kafka.test.TestUtils.waitForCondition(TestUtils.java:342)
	at org.apache.kafka.connect.integration.ExampleConnectIntegrationTest.testProduceConsumeConnector(ExampleConnectIntegrationTest.java:123)

and

java.util.concurrent.ExecutionException: org.apache.kafka.common.errors.CoordinatorNotAvailableException: The coordinator is not available.
	at org.apache.kafka.common.internals.KafkaFutureImpl.wrapAndThrow(KafkaFutureImpl.java:45)
	at org.apache.kafka.common.internals.KafkaFutureImpl.access$000(KafkaFutureImpl.java:32)
	at org.apache.kafka.common.internals.KafkaFutureImpl$SingleWaiter.await(KafkaFutureImpl.java:89)
	at org.apache.kafka.common.internals.KafkaFutureImpl.get(KafkaFutureImpl.java:260)
	at kafka.admin.ConsumerGroupCommand$ConsumerGroupService.resetOffsets(ConsumerGroupCommand.scala:306)
	at kafka.admin.ResetConsumerGroupOffsetTest.testResetOffsetsNotExistingGroup(ResetConsumerGroupOffsetTest.scala:89)

https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/19220/

java.util.concurrent.ExecutionException: org.apache.kafka.common.errors.TimeoutException: Aborted due to timeout.
	at org.apache.kafka.common.internals.KafkaFutureImpl.wrapAndThrow(KafkaFutureImpl.java:45)
	at org.apache.kafka.common.internals.KafkaFutureImpl.access$000(KafkaFutureImpl.java:32)
	at org.apache.kafka.common.internals.KafkaFutureImpl$SingleWaiter.await(KafkaFutureImpl.java:89)
	at org.apache.kafka.common.internals.KafkaFutureImpl.get(KafkaFutureImpl.java:260)
	at kafka.admin.ConsumerGroupCommand$ConsumerGroupService.collectGroupState(ConsumerGroupCommand.scala:389)
	at kafka.admin.ConsumerGroupCommand$ConsumerGroupService.describeGroup(ConsumerGroupCommand.scala:257)
	at kafka.admin.DescribeConsumerGroupTest$$anonfun$testDescribeWithConsumersWithoutAssignedPartitions$1$$anonfun$16$$anonfun$17.apply$mcV$sp(DescribeConsumerGroupTest.scala:344)
	at kafka.admin.DescribeConsumerGroupTest$$anonfun$testDescribeWithConsumersWithoutAssignedPartitions$1$$anonfun$16$$anonfun$17.apply(DescribeConsumerGroupTest.scala:344)
	at kafka.admin.DescribeConsumerGroupTest$$anonfun$testDescribeWithConsumersWithoutAssignedPartitions$1$$anonfun$16$$anonfun$17.apply(DescribeConsumerGroupTest.scala:344)
	at scala.util.DynamicVariable.withValue(DynamicVariable.scala:58)
	at scala.Console$.withErr(Console.scala:92)
	at kafka.utils.TestUtils$$anonfun$grabConsoleOutputAndError$1.apply$mcV$sp(TestUtils.scala:1419)
	at kafka.utils.TestUtils$$anonfun$grabConsoleOutputAndError$1.apply(TestUtils.scala:1419)
	at kafka.utils.TestUtils$$anonfun$grabConsoleOutputAndError$1.apply(TestUtils.scala:1419)
	at scala.util.DynamicVariable.withValue(DynamicVariable.scala:58)
	at scala.Console$.withOut(Console.scala:65)
	at kafka.utils.TestUtils$.grabConsoleOutputAndError(TestUtils.scala:1419)
	at kafka.admin.DescribeConsumerGroupTest$$anonfun$testDescribeWithConsumersWithoutAssignedPartitions$1$$anonfun$16.apply$mcZ$sp(DescribeConsumerGroupTest.scala:344)
	at kafka.utils.TestUtils$.waitUntilTrue(TestUtils.scala:763)
	at kafka.admin.DescribeConsumerGroupTest$$anonfun$testDescribeWithConsumersWithoutAssignedPartitions$1.apply(DescribeConsumerGroupTest.scala:343)
	at kafka.admin.DescribeConsumerGroupTest$$anonfun$testDescribeWithConsumersWithoutAssignedPartitions$1.apply(DescribeConsumerGroupTest.scala:336)
	at scala.collection.IndexedSeqOptimized$class.foreach(IndexedSeqOptimized.scala:33)
	at scala.collection.mutable.ArrayOps$ofRef.foreach(ArrayOps.scala:186)
	at kafka.admin.DescribeConsumerGroupTest.testDescribeWithConsumersWithoutAssignedPartitions(DescribeConsumerGroupTest.scala:336)

Retest this please.

@mjsax
Copy link
Copy Markdown
Member Author

mjsax commented Feb 3, 2019

We now have 2.2 branch -- wondering if this should only go into 2.2 but not to trunk?

@vvcephei
Copy link
Copy Markdown
Contributor

vvcephei commented Feb 5, 2019

@mjsax , yes, I think that's right.

Copy link
Copy Markdown
Member

@bbejeck bbejeck left a comment

Choose a reason for hiding this comment

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

Thanks @mjsax for the clean solution, LGTM.

We kow have 2.2 branch -- wondering if this should only go into 2.2 but not to trunk?

That sounds right to me.

Copy link
Copy Markdown
Contributor

@guozhangwang guozhangwang left a comment

Choose a reason for hiding this comment

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

LGTM. Please feel free to merge.

@mjsax mjsax changed the base branch from trunk to 2.2 February 9, 2019 01:19
@mjsax
Copy link
Copy Markdown
Member Author

mjsax commented Feb 9, 2019

Retest this please.

@mjsax
Copy link
Copy Markdown
Member Author

mjsax commented Feb 9, 2019

https://builds.apache.org/job/kafka-pr-jdk11-scala2.12/2225/

kafka.admin.ReassignPartitionsClusterTest > shouldExecuteThrottledReassignment FAILED
00:25:58     java.lang.AssertionError: Expected replication to be < 10000 but was 10521
00:25:58         at org.junit.Assert.fail(Assert.java:88)
00:25:58         at org.junit.Assert.assertTrue(Assert.java:41)
00:25:58         at 

https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/19320/testReport/junit/kafka.api/TransactionsTest/testFencingOnTransactionExpiration/

java.lang.AssertionError: expected:<1> but was:<0>
	at org.junit.Assert.fail(Assert.java:88)
	at org.junit.Assert.failNotEquals(Assert.java:834)
	at org.junit.Assert.assertEquals(Assert.java:645)
	at org.junit.Assert.assertEquals(Assert.java:631)
	at kafka.api.TransactionsTest.testFencingOnTransactionExpiration(TransactionsTest.scala:510)

Retest this please

@mjsax
Copy link
Copy Markdown
Member Author

mjsax commented Feb 10, 2019

https://builds.apache.org/job/kafka-pr-jdk11-scala2.12/2227/

kafka.admin.ReassignPartitionsClusterTest > shouldExecuteThrottledReassignment FAILED
12:24:24     java.lang.AssertionError: Expected replication to be < 10000 but was 10175
12:24:24         at org.junit.Assert.fail(Assert.java:88)
12:24:24         at org.junit.Assert.assertTrue(Assert.java:41)
12:24:24         at 

@ijuma This test failed two times in a row for Java11. Is it a know issue and is anybody working on it?

Java 8 passed.

Retest this please.

@ijuma
Copy link
Copy Markdown
Member

ijuma commented Feb 10, 2019

@mjsax We thought we had fixed it with a PR from @stanislavkozlovski. Looks like we did not. It's not Java 11 specific.

@mjsax
Copy link
Copy Markdown
Member Author

mjsax commented Feb 10, 2019

Was the fix cherry-picked to 2.2 branch (this PR is not for trunk)? Maybe I need to rebase the PR?

@mjsax
Copy link
Copy Markdown
Member Author

mjsax commented Feb 10, 2019

https://builds.apache.org/job/kafka-pr-jdk11-scala2.12/2230/

java.lang.AssertionError: Expected replication to be < 10000 but was 10010
	at org.junit.Assert.fail(Assert.java:88)
	at org.junit.Assert.assertTrue(Assert.java:41)
	at kafka.admin.ReassignPartitionsClusterTest.shouldExecuteThrottledReassignment(ReassignPartitionsClusterTest.scala:304)

I was looking into commit history. Seems you refer to 66129b1 ? This is only in trunk but not in 2.2. Can you confirm that we should cherry-pick it to 2.2 to stabilize the failing test?

\cc @ijuma @stanislavkozlovski

@ijuma
Copy link
Copy Markdown
Member

ijuma commented Feb 10, 2019

@mjsax sounds good.

@mjsax mjsax force-pushed the kafka-3522-rocksdb-format-make-public-api-private branch from c49b886 to bbeeada Compare February 10, 2019 19:40
@mjsax
Copy link
Copy Markdown
Member Author

mjsax commented Feb 10, 2019

Cherry-picked the fix to 2.2 and rebased this PR.

@mjsax
Copy link
Copy Markdown
Member Author

mjsax commented Feb 11, 2019

https://builds.apache.org/job/kafka-pr-jdk8-scala2.11/19332/

org.junit.runners.model.TestTimedOutException: test timed out after 120000 milliseconds
	at java.lang.Object.wait(Native Method)
	at java.lang.Object.wait(Object.java:502)
	at org.apache.kafka.common.internals.KafkaFutureImpl$SingleWaiter.await(KafkaFutureImpl.java:92)
	at org.apache.kafka.common.internals.KafkaFutureImpl.get(KafkaFutureImpl.java:260)
	at kafka.utils.TestUtils$.assertFutureExceptionTypeEquals(TestUtils.scala:1429)
	at kafka.api.AdminClientIntegrationTest.testMinimumRequestTimeouts(AdminClientIntegrationTest.scala:1071)

Retest this please.

@mjsax mjsax merged commit 692463f into apache:2.2 Feb 11, 2019
@mjsax mjsax deleted the kafka-3522-rocksdb-format-make-public-api-private branch February 11, 2019 09:16
@mjsax mjsax added the kip Requires or implements a KIP label Jun 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

kip Requires or implements a KIP streams

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants