HDDS-6109 Preserve the underlying exception raised in client lib.

[kerneltime]

What changes were proposed in this pull request?

The underlying exceptions should not be mapped to RuntimeException.
This results in higher layers not retrying on valid failure scenarios.

What is the link to the Apache JIRA

https://issues.apache.org/jira/browse/HDDS-6109?filter=-1

How was this patch tested?

Included integration tests that introduce a single and dual replica failure both of which should result in the unflushed data to be written to a new location.

[kerneltime]

Ref: 565972c #2795

[kerneltime]

cc @hanishakoneru

[bharatviswa504]

Overall LGTM.
Thank You @kerneltime for the root causing and fixing this issue.

[bharatviswa504]

There are lot of space changes, is it intentional?? If it is not, can you revert those.

[adoroszlai]

Thanks @kerneltime for the patch.

[adoroszlai]

@kerneltime Do we need to restart SCM and datanodes between test cases? If so, do need as many as 10 datanodes?

The previous change in this test class added:

ozone/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestContainerStateMachineFailures.java

Lines 174 to 182 in 3eb7235

	@BeforeEach
	public void restartDatanode()
	throws InterruptedException, TimeoutException, AuthenticationException,
	IOException {
	for (int i=0; i < cluster.getHddsDatanodes().size(); i++) {
	cluster.restartHddsDatanode(i, true);
	}
	cluster.restartStorageContainerManager(true);
	}

@BeforeEach is from JUnit5, but the rest of the class uses JUnit4 annotations. Thus restartDatanode() is never called. The corresponding JUnit4 annotation is @Before. (Note: I plan to change this test to JUnit5 in HDDS-6095.)

Fixing the annotation would increase test execution time significantly from 100 seconds. I don't know how much, because Surefire fork gets killed at 20 minutes.

Also, if the annotation was fixed, SCM and datanodes would be restarted even before the first test case, which is unnecessary. It may be better to change the cluster to per-test.

master with per-test cluster:

Tests run: 6, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 236.074 s - in org.apache.hadoop.ozone.client.rpc.TestContainerStateMachineFailures

This patch with per-test cluster:

Tests run: 8, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 298.905 s - in org.apache.hadoop.ozone.client.rpc.TestContainerStateMachineFailures

This patch with per-test cluster with 6 datanodes (not much improvement here):

Tests run: 8, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 292.42 s - in org.apache.hadoop.ozone.client.rpc.TestContainerStateMachineFailures

[kerneltime]

@kerneltime Do we need to restart SCM and datanodes between test cases? If so, do need as many as 10 datanodes?

The previous change in this test class added:

ozone/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestContainerStateMachineFailures.java

Lines 174 to 182 in 3eb7235

	@BeforeEach
	public void restartDatanode()
	throws InterruptedException, TimeoutException, AuthenticationException,
	IOException {
	for (int i=0; i < cluster.getHddsDatanodes().size(); i++) {
	cluster.restartHddsDatanode(i, true);
	}
	cluster.restartStorageContainerManager(true);
	}

@BeforeEach is from JUnit5, but the rest of the class uses JUnit4 annotations. Thus restartDatanode() is never called. The corresponding JUnit4 annotation is @Before. (Note: I plan to change this test to JUnit5 in HDDS-6095.)

Fixing the annotation would increase test execution time significantly from 100 seconds. I don't know how much, because Surefire fork gets killed at 20 minutes.

Also, if the annotation was fixed, SCM and datanodes would be restarted even before the first test case, which is unnecessary. It may be better to change the cluster to per-test.

master with per-test cluster:

Tests run: 6, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 236.074 s - in org.apache.hadoop.ozone.client.rpc.TestContainerStateMachineFailures

This patch with per-test cluster:

Tests run: 8, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 298.905 s - in org.apache.hadoop.ozone.client.rpc.TestContainerStateMachineFailures

This patch with per-test cluster with 6 datanodes (not much improvement here):

Tests run: 8, Failures: 0, Errors: 0, Skipped: 0, Time elapsed: 292.42 s - in org.apache.hadoop.ozone.client.rpc.TestContainerStateMachineFailures

Let me look more deeper on the performance.
These tests had 10 but I don't think we need 10 DNs. We would need more than 3. Let me fix the annotation and look into performance. Without the restart, the failures induced can lead to other tests being flaky.

[kerneltime]

Thanks @adoroszlai for the review and catching the version for the annotation.
I updated the tests and ran them multiple times to make sure they are not flaky. Hopefully that is enough, will see what happens with the CI run. This should avoid a massive increase in overall test time as well as test the intended code path.

As long as the container that the objects were initially flushed to, is moved to state other than open, the retry logic kicks in to find a different pipeline and complete the write and flush. Thus, expecting a count of 2 for the number of locations seems valid. Before the fix if the exception raised was inherited from IOException the retry logic would not kick in and the flush()/close() would fail.

[adoroszlai]

Thanks @kerneltime for updating the patch.

[adoroszlai]

Thanks @kerneltime for the patch, @bharatviswa504 for the review.