Skip to content

HDDS-6401. Fix flaky TestFilePerBlockStrategy.testWriteAndReadChunkMultipleTimes#3152

Merged
adoroszlai merged 2 commits intoapache:masterfrom
kuenishi:remove-writetime-check
Mar 2, 2022
Merged

HDDS-6401. Fix flaky TestFilePerBlockStrategy.testWriteAndReadChunkMultipleTimes#3152
adoroszlai merged 2 commits intoapache:masterfrom
kuenishi:remove-writetime-check

Conversation

@kuenishi
Copy link
Copy Markdown
Contributor

@kuenishi kuenishi commented Mar 2, 2022

What changes were proposed in this pull request?

When ChunkUtils.writeData() finishes too fast, elapsed time for writing data in test case becomes zero because it's resolution is a bit rough, in milliseconds. That leads to the failure of test assertion that checks elapsed time being >0. Here, the check must be on written bytes and ops.

I think this failure does not affect any released version, but current master HEAD is deterministically failing in the unit test due to this issue.

2022-03-02 19:29:26,493 [main] INFO  volume.HddsVolume (HddsVolume.java:<init>(120)) - Creating HddsVolume: /tmp/junit871563182678035731/hdds of storage type : null capacity : 9223372036854775807
2022-03-02 19:29:27,280 [main] WARN  util.NativeCodeLoader (NativeCodeLoader.java:<clinit>(60)) - Unable to load native-hadoop library for your platform... using builtin-java classes where applicable

java.lang.AssertionError
	at org.junit.Assert.fail(Assert.java:87)
	at org.junit.Assert.assertTrue(Assert.java:42)
	at org.junit.Assert.assertTrue(Assert.java:53)
	at org.apache.hadoop.ozone.container.keyvalue.impl.CommonChunkManagerTestCases.testWriteAndReadChunkMultipleTimes(CommonChunkManagerTestCases.java:228)
	at sun.reflect.NativeMethodAccessorImpl.invoke0(Native Method)
	at sun.reflect.NativeMethodAccessorImpl.invoke(NativeMethodAccessorImpl.java:62)
	at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
....

What is the link to the Apache JIRA

HDDS-6401

How was this patch tested?

This is a test code fix

Copy link
Copy Markdown
Contributor

@adoroszlai adoroszlai left a comment

Choose a reason for hiding this comment

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

Thanks @kuenishi for the patch. I have never seen this specific assertion fail in CI. But based on experience with similar time-based tests I can imagine this might fail in some environments.

Removing the assertion is OK. But we can omit the new assertions, as bytes and count are already checked in checkWriteIOStats:

protected void checkWriteIOStats(long length, long opCount) {
VolumeIOStats volumeIOStats = hddsVolume.getVolumeIOStats();
assertEquals(length, volumeIOStats.getWriteBytes());
assertEquals(opCount, volumeIOStats.getWriteOpCount());
}

…ozone/container/keyvalue/impl/CommonChunkManagerTestCases.java

Co-authored-by: Doroszlai, Attila <6454655+adoroszlai@users.noreply.github.com>
@kuenishi
Copy link
Copy Markdown
Contributor Author

kuenishi commented Mar 2, 2022

That makes sense, thank you. I committed your suggestion.

@kuenishi kuenishi requested a review from adoroszlai March 2, 2022 14:37
@adoroszlai adoroszlai merged commit e9722b7 into apache:master Mar 2, 2022
@kuenishi kuenishi deleted the remove-writetime-check branch March 3, 2022 00:11
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.

2 participants