Skip to content

HDDS-16100. Parameterize TestChunkInputStream - #11117

Open
adoroszlai wants to merge 3 commits into
apache:masterfrom
adoroszlai:HDDS-16100
Open

HDDS-16100. Parameterize TestChunkInputStream#11117
adoroszlai wants to merge 3 commits into
apache:masterfrom
adoroszlai:HDDS-16100

Conversation

@adoroszlai

Copy link
Copy Markdown
Contributor

What changes were proposed in this pull request?

  • Make TestLocalChunkInputStream extend InpuStreamTests instead of TestChunkInputStream
  • Parameterize TestChunkInputStream to also test with short-circuit read enabled
  • Keep only testFallbackToGrpc in TestLocalChunkInputStream
  • Remove unnecessary cluster creation from TestStreamBlockInputStream

https://issues.apache.org/jira/browse/HDDS-16100

How was this patch tested?

Without native lib:

Tests run: 2, Failures: 0, Errors: 0, Skipped: 2, Time elapsed: 0.037 s -- in org.apache.hadoop.ozone.client.rpc.read.TestChunkInputStream
Tests run: 2, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 19.91 s -- in org.apache.hadoop.ozone.client.rpc.read.TestChunkInputStream
Tests run: 0, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 37.15 s -- in org.apache.hadoop.ozone.client.rpc.read.TestChunkInputStream
Tests run: 4, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 64.82 s -- in org.apache.hadoop.ozone.client.rpc.read.TestKeyInputStream
Tests run: 0, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 0.063 s -- in org.apache.hadoop.ozone.client.rpc.read.TestLocalChunkInputStream
Tests run: 3, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 52.97 s -- in org.apache.hadoop.ozone.client.rpc.read.TestStreamBlockInputStream

With native lib:

Tests run: 2, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 21.79 s -- in org.apache.hadoop.ozone.client.rpc.read.TestChunkInputStream
Tests run: 2, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 12.90 s -- in org.apache.hadoop.ozone.client.rpc.read.TestChunkInputStream
Tests run: 0, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 55.68 s -- in org.apache.hadoop.ozone.client.rpc.read.TestChunkInputStream
Tests run: 4, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 87.33 s -- in org.apache.hadoop.ozone.client.rpc.read.TestKeyInputStream
Tests run: 1, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 35.89 s -- in org.apache.hadoop.ozone.client.rpc.read.TestLocalChunkInputStream
Tests run: 3, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 92.17 s -- in org.apache.hadoop.ozone.client.rpc.read.TestStreamBlockInputStream

https://github.com/adoroszlai/ozone/actions/runs/32855529592

.setStreamBufferMaxSize(MAX_FLUSH_SIZE)
.applyTo(conf);

enableShortCircuitRead(dir, conf);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could we keep short-circuit setup opt-in for TestChunkInputStream and TestLocalChunkInputStream? Enabling it in the shared InputStreamTests setup also changes TestKeyInputStream and TestStreamBlockInputStream to use LocalChunkInputStream when the native library is available, so their existing gRPC coverage is lost.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks, updated.

testCloseReleasesBuffers(bucket);
}

assertEquals(useShortCircuitRead, localChunkInputStreamLog.getOutput()

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit: Could we use AssertJ contains / doesNotContain here? This would make a failure show which log content selected the wrong client path.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We would have to mirror the statements for the two cases, so I prefer assertEquals here.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ok , got it

@rich7420

Copy link
Copy Markdown
Contributor

@adoroszlai thanks for the patch!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants