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

KAFKA-6234: Increased timeout value for lowWatermark response to avoid test failing occasionally #4238

Merged
merged 4 commits into from
Apr 12, 2018
+10 −11
Merged

KAFKA-6234: Increased timeout value for lowWatermark response to avoid test failing occasionally #4238

Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
5bd2cdc
KAFKA-6234: Removed timeout from get call that caused the test to fai…
soenkeliebau Nov 20, 2017
8437bfd
Addressed Jason's comments: Used Option instead of Long.MinValue and …
soenkeliebau Apr 12, 2018
1c5acf3
Refactored assert Statements to give better error message and used So…
soenkeliebau Apr 12, 2018
7f93009
Add space before type declaration
Apr 12, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
21 changes: 10 additions & 11 deletions core/src/test/scala/integration/kafka/api/AdminClientIntegrationTest.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,7 +38,7 @@ import org.apache.kafka.common.{KafkaFuture, TopicPartition, TopicPartitionRepli
import org.apache.kafka.common.acl._
import org.apache.kafka.common.config.ConfigResource
import org.apache.kafka.common.errors._
import org.junit.{After, Before, Ignore, Rule, Test}
import org.junit.{After, Before, Rule, Test}
import org.apache.kafka.common.requests.{DeleteRecordsRequest, MetadataResponse}
import org.apache.kafka.common.resource.{Resource, ResourceType}
import org.junit.rules.Timeout
Expand Down Expand Up @@ -733,7 +733,6 @@ class AdminClientIntegrationTest extends IntegrationTestHarness with Logging {
}

@Test
@Ignore // Disabled temporarily until flakiness is resolved
def testLogStartOffsetCheckpoint(): Unit = {
TestUtils.createTopic(zkUtils, topic, 2, serverCount, servers)

Expand All @@ -743,8 +742,8 @@ class AdminClientIntegrationTest extends IntegrationTestHarness with Logging {

sendRecords(producers.head, 10, topicPartition)
var result = client.deleteRecords(Map(topicPartition -> RecordsToDelete.beforeOffset(5L)).asJava)
var lowWatermark = result.lowWatermarks().get(topicPartition).get().lowWatermark()
assertEquals(5L, lowWatermark)
var lowWatermark = Option(result.lowWatermarks.get(topicPartition).get.lowWatermark)
Copy link

Choose a reason for hiding this comment

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

nit: Could we use Some? Same below.

Copy link
Contributor Author

@soenkeliebau soenkeliebau Apr 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

I initially used Some(..) but subsequently learned that Some(null) is not None, however Option(null) is None and should be preferred so thought that it is better to use Option here (not that I'd expect lowWatermark to return null, but you never really know).

But I really just read about this today, so this might be misguided :)

Copy link

Choose a reason for hiding this comment

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

Yeah, that makes sense, but in this case lowWatermark is a long which cannot be null.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fair point, I'll push a commit with this fixed and @ijuma 's concern addressed.

assertTrue(lowWatermark.contains(5L))
Copy link
Member

Choose a reason for hiding this comment

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

It's a bit better to use assertEquald(Some(5), lowWatermark) so that you get a good error message.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, I'll change that once Jason has chimed in so that I can address his comments as well if he has any.


for (i <- 0 until serverCount) {
killBroker(i)
Expand All @@ -759,16 +758,16 @@ class AdminClientIntegrationTest extends IntegrationTestHarness with Logging {
// Need to retry if leader is not available for the partition
result = client.deleteRecords(Map(topicPartition -> RecordsToDelete.beforeOffset(0L)).asJava)

lowWatermark = None
val future = result.lowWatermarks().get(topicPartition)
try {
lowWatermark = future.get(1000L, TimeUnit.MILLISECONDS).lowWatermark()
lowWatermark == 5L
lowWatermark = Option(future.get.lowWatermark)
lowWatermark.contains(5L)
} catch {
case e: LeaderNotAvailableException => false
}

}, "Expected low watermark of the partition to be 5L")

case e: ExecutionException if e.getCause.isInstanceOf[LeaderNotAvailableException] ||
e.getCause.isInstanceOf[NotLeaderForPartitionException] => false
}
}, s"Expected low watermark of the partition to be 5 but got ${lowWatermark.getOrElse("no response within the timeout")}")
client.close()
}

Expand Down