You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@ozone.apache.org by GitBox <gi...@apache.org> on 2022/01/13 23:34:57 UTC
[GitHub] [ozone] kerneltime opened a new pull request #2989: HDDS-6109 Preserve the underlying exception raised in client lib.
kerneltime opened a new pull request #2989:
URL: https://github.com/apache/ozone/pull/2989
## 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai merged pull request #2989: HDDS-6109 Preserve the underlying exception raised in client lib.
Posted by GitBox <gi...@apache.org>.
adoroszlai merged pull request #2989:
URL: https://github.com/apache/ozone/pull/2989
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] kerneltime edited a comment on pull request #2989: HDDS-6109 Preserve the underlying exception raised in client lib.
Posted by GitBox <gi...@apache.org>.
kerneltime edited a comment on pull request #2989:
URL: https://github.com/apache/ozone/pull/2989#issuecomment-1016075709
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on a change in pull request #2989: HDDS-6109 Preserve the underlying exception raised in client lib.
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on a change in pull request #2989:
URL: https://github.com/apache/ozone/pull/2989#discussion_r784594546
##########
File path: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestContainerStateMachineFailures.java
##########
@@ -677,4 +682,133 @@ public void testWriteStateMachineDataIdempotencyWithClosedContainer()
r2.run();
}
+
+ @Test
+ public void testContainerStateMachineSingleFailureRetry()
+ throws Exception {
+ OzoneOutputStream key =
+ objectStore.getVolume(volumeName).getBucket(bucketName)
+ .createKey("ratis1", 1024, ReplicationType.RATIS,
+ ReplicationFactor.THREE, new HashMap<>());
+
+ key.write("ratis".getBytes(UTF_8));
+ key.flush();
+ key.write("ratis".getBytes(UTF_8));
+ key.write("ratis".getBytes(UTF_8));
+
+ KeyOutputStream groupOutputStream = (KeyOutputStream) key.
+ getOutputStream();
+ List<OmKeyLocationInfo> locationInfoList =
+ groupOutputStream.getLocationInfoList();
+ Assert.assertEquals(1, locationInfoList.size());
+
+ OmKeyLocationInfo omKeyLocationInfo = locationInfoList.get(0);
+
+ Set<HddsDatanodeService> datanodeSet =
+ TestHelper.getDatanodeServices(cluster,
+ omKeyLocationInfo.getPipeline());
+
+ UUID leader = omKeyLocationInfo.getPipeline().getLeaderId();
+ for (HddsDatanodeService dn : datanodeSet) {
+ UUID dnUuid = dn.getDatanodeDetails().getUuid();
+ if (!dnUuid.equals(leader)) {
+ ContainerData containerData =
+ dn.getDatanodeStateMachine()
+ .getContainer().getContainerSet()
+ .getContainer(omKeyLocationInfo.getContainerID())
+ .getContainerData();
+ Assert.assertTrue(containerData instanceof KeyValueContainerData);
+ KeyValueContainerData keyValueContainerData =
+ (KeyValueContainerData) containerData;
+ FileUtil.fullyDelete(new File(keyValueContainerData.getChunksPath()));
+ break;
+ }
+ }
+ try {
+ key.close();
+ } catch (Exception ioe) {
+ // Should not fail..
+ Assert.fail("Exception " + ioe.getMessage());
+ }
+ validateData("ratis1", 2, "ratisratisratis");
+ }
+
+ @Test
+ public void testContainerStateMachineDualFailureRetry()
Review comment:
`testContainerStateMachineSingleFailureRetry` and `testContainerStateMachineDualFailureRetry` are almost the same. Can you please reduce code duplication to make the rather subtle difference more visible?
Something along the lines:
```
OzoneOutputStream key = writeKey("ratis1");
deleteFollowerContainers(getKeyLocationInfo(key), 1);
closeKey(key);
validateData("ratis1", 2, "ratisratisratis");
```
BTW, Is 2 the expected `locationCount` due to the call to `key.flush()`?
##########
File path: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestContainerStateMachineFailures.java
##########
@@ -677,4 +682,133 @@ public void testWriteStateMachineDataIdempotencyWithClosedContainer()
r2.run();
}
+
+ @Test
+ public void testContainerStateMachineSingleFailureRetry()
+ throws Exception {
+ OzoneOutputStream key =
+ objectStore.getVolume(volumeName).getBucket(bucketName)
+ .createKey("ratis1", 1024, ReplicationType.RATIS,
+ ReplicationFactor.THREE, new HashMap<>());
+
+ key.write("ratis".getBytes(UTF_8));
+ key.flush();
+ key.write("ratis".getBytes(UTF_8));
+ key.write("ratis".getBytes(UTF_8));
+
+ KeyOutputStream groupOutputStream = (KeyOutputStream) key.
+ getOutputStream();
+ List<OmKeyLocationInfo> locationInfoList =
+ groupOutputStream.getLocationInfoList();
+ Assert.assertEquals(1, locationInfoList.size());
+
+ OmKeyLocationInfo omKeyLocationInfo = locationInfoList.get(0);
+
+ Set<HddsDatanodeService> datanodeSet =
+ TestHelper.getDatanodeServices(cluster,
+ omKeyLocationInfo.getPipeline());
+
+ UUID leader = omKeyLocationInfo.getPipeline().getLeaderId();
+ for (HddsDatanodeService dn : datanodeSet) {
+ UUID dnUuid = dn.getDatanodeDetails().getUuid();
+ if (!dnUuid.equals(leader)) {
+ ContainerData containerData =
+ dn.getDatanodeStateMachine()
+ .getContainer().getContainerSet()
+ .getContainer(omKeyLocationInfo.getContainerID())
+ .getContainerData();
+ Assert.assertTrue(containerData instanceof KeyValueContainerData);
+ KeyValueContainerData keyValueContainerData =
+ (KeyValueContainerData) containerData;
+ FileUtil.fullyDelete(new File(keyValueContainerData.getChunksPath()));
+ break;
+ }
+ }
+ try {
+ key.close();
+ } catch (Exception ioe) {
+ // Should not fail..
+ Assert.fail("Exception " + ioe.getMessage());
+ }
+ validateData("ratis1", 2, "ratisratisratis");
+ }
+
+ @Test
+ public void testContainerStateMachineDualFailureRetry()
+ throws Exception {
+ OzoneOutputStream key =
+ objectStore.getVolume(volumeName).getBucket(bucketName)
+ .createKey("ratis2", 1024, ReplicationType.RATIS,
+ ReplicationFactor.THREE, new HashMap<>());
+
+ key.write("ratis".getBytes(UTF_8));
+ key.flush();
+ key.write("ratis".getBytes(UTF_8));
+ key.write("ratis".getBytes(UTF_8));
+
+ KeyOutputStream groupOutputStream = (KeyOutputStream) key.
+ getOutputStream();
+ List<OmKeyLocationInfo> locationInfoList =
+ groupOutputStream.getLocationInfoList();
+ Assert.assertEquals(1, locationInfoList.size());
+
+ OmKeyLocationInfo omKeyLocationInfo = locationInfoList.get(0);
+
+ Set<HddsDatanodeService> datanodeSet =
+ TestHelper.getDatanodeServices(cluster,
+ omKeyLocationInfo.getPipeline());
+
+ UUID leader = omKeyLocationInfo.getPipeline().getLeaderId();
+ for (HddsDatanodeService dn : datanodeSet) {
+ UUID dnUuid = dn.getDatanodeDetails().getUuid();
+ if (!dnUuid.equals(leader)) {
+ ContainerData containerData =
+ dn.getDatanodeStateMachine()
+ .getContainer().getContainerSet()
+ .getContainer(omKeyLocationInfo.getContainerID())
+ .getContainerData();
+ Assert.assertTrue(containerData instanceof KeyValueContainerData);
+ KeyValueContainerData keyValueContainerData =
+ (KeyValueContainerData) containerData;
+ FileUtil.fullyDelete(new File(keyValueContainerData.getChunksPath()));
+ }
+ }
+ try {
+ key.close();
+ } catch (Exception ioe) {
+ // Should not fail..
+ Assert.fail("Exception " + ioe.getMessage());
+ }
+ validateData("ratis2", 2, "ratisratisratis");
+ }
+
+ private void validateData(String key, int locationCount, String payload) {
+ OmKeyArgs omKeyArgs = new OmKeyArgs.Builder()
+ .setVolumeName(volumeName)
+ .setBucketName(bucketName)
+ .setKeyName(key)
+ .setRefreshPipeline(true)
+ .build();
+ OmKeyInfo keyInfo = null;
+ try {
+ keyInfo = cluster.getOzoneManager().lookupKey(omKeyArgs);
+ Assert.assertEquals(
+ keyInfo.getLatestVersionLocations().getLocationListCount(),
+ locationCount);
Review comment:
Nit: actual and expected values are swapped.
```suggestion
Assert.assertEquals(locationCount,
keyInfo.getLatestVersionLocations().getLocationListCount());
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on pull request #2989: HDDS-6109 Preserve the underlying exception raised in client lib.
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on pull request #2989:
URL: https://github.com/apache/ozone/pull/2989#issuecomment-1018247490
Thanks @kerneltime for the patch, @bharatviswa504 for the review.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] kerneltime commented on a change in pull request #2989: HDDS-6109 Preserve the underlying exception raised in client lib.
Posted by GitBox <gi...@apache.org>.
kerneltime commented on a change in pull request #2989:
URL: https://github.com/apache/ozone/pull/2989#discussion_r786409893
##########
File path: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestContainerStateMachineFailures.java
##########
@@ -677,4 +682,133 @@ public void testWriteStateMachineDataIdempotencyWithClosedContainer()
r2.run();
}
+
+ @Test
+ public void testContainerStateMachineSingleFailureRetry()
+ throws Exception {
+ OzoneOutputStream key =
+ objectStore.getVolume(volumeName).getBucket(bucketName)
+ .createKey("ratis1", 1024, ReplicationType.RATIS,
+ ReplicationFactor.THREE, new HashMap<>());
+
+ key.write("ratis".getBytes(UTF_8));
+ key.flush();
+ key.write("ratis".getBytes(UTF_8));
+ key.write("ratis".getBytes(UTF_8));
+
+ KeyOutputStream groupOutputStream = (KeyOutputStream) key.
+ getOutputStream();
+ List<OmKeyLocationInfo> locationInfoList =
+ groupOutputStream.getLocationInfoList();
+ Assert.assertEquals(1, locationInfoList.size());
+
+ OmKeyLocationInfo omKeyLocationInfo = locationInfoList.get(0);
+
+ Set<HddsDatanodeService> datanodeSet =
+ TestHelper.getDatanodeServices(cluster,
+ omKeyLocationInfo.getPipeline());
+
+ UUID leader = omKeyLocationInfo.getPipeline().getLeaderId();
+ for (HddsDatanodeService dn : datanodeSet) {
+ UUID dnUuid = dn.getDatanodeDetails().getUuid();
+ if (!dnUuid.equals(leader)) {
+ ContainerData containerData =
+ dn.getDatanodeStateMachine()
+ .getContainer().getContainerSet()
+ .getContainer(omKeyLocationInfo.getContainerID())
+ .getContainerData();
+ Assert.assertTrue(containerData instanceof KeyValueContainerData);
+ KeyValueContainerData keyValueContainerData =
+ (KeyValueContainerData) containerData;
+ FileUtil.fullyDelete(new File(keyValueContainerData.getChunksPath()));
+ break;
+ }
+ }
+ try {
+ key.close();
+ } catch (Exception ioe) {
+ // Should not fail..
+ Assert.fail("Exception " + ioe.getMessage());
+ }
+ validateData("ratis1", 2, "ratisratisratis");
+ }
+
+ @Test
+ public void testContainerStateMachineDualFailureRetry()
+ throws Exception {
+ OzoneOutputStream key =
+ objectStore.getVolume(volumeName).getBucket(bucketName)
+ .createKey("ratis2", 1024, ReplicationType.RATIS,
+ ReplicationFactor.THREE, new HashMap<>());
+
+ key.write("ratis".getBytes(UTF_8));
+ key.flush();
+ key.write("ratis".getBytes(UTF_8));
+ key.write("ratis".getBytes(UTF_8));
+
+ KeyOutputStream groupOutputStream = (KeyOutputStream) key.
+ getOutputStream();
+ List<OmKeyLocationInfo> locationInfoList =
+ groupOutputStream.getLocationInfoList();
+ Assert.assertEquals(1, locationInfoList.size());
+
+ OmKeyLocationInfo omKeyLocationInfo = locationInfoList.get(0);
+
+ Set<HddsDatanodeService> datanodeSet =
+ TestHelper.getDatanodeServices(cluster,
+ omKeyLocationInfo.getPipeline());
+
+ UUID leader = omKeyLocationInfo.getPipeline().getLeaderId();
+ for (HddsDatanodeService dn : datanodeSet) {
+ UUID dnUuid = dn.getDatanodeDetails().getUuid();
+ if (!dnUuid.equals(leader)) {
+ ContainerData containerData =
+ dn.getDatanodeStateMachine()
+ .getContainer().getContainerSet()
+ .getContainer(omKeyLocationInfo.getContainerID())
+ .getContainerData();
+ Assert.assertTrue(containerData instanceof KeyValueContainerData);
+ KeyValueContainerData keyValueContainerData =
+ (KeyValueContainerData) containerData;
+ FileUtil.fullyDelete(new File(keyValueContainerData.getChunksPath()));
+ }
+ }
+ try {
+ key.close();
+ } catch (Exception ioe) {
+ // Should not fail..
+ Assert.fail("Exception " + ioe.getMessage());
+ }
+ validateData("ratis2", 2, "ratisratisratis");
+ }
+
+ private void validateData(String key, int locationCount, String payload) {
+ OmKeyArgs omKeyArgs = new OmKeyArgs.Builder()
+ .setVolumeName(volumeName)
+ .setBucketName(bucketName)
+ .setKeyName(key)
+ .setRefreshPipeline(true)
+ .build();
+ OmKeyInfo keyInfo = null;
+ try {
+ keyInfo = cluster.getOzoneManager().lookupKey(omKeyArgs);
+ Assert.assertEquals(
+ keyInfo.getLatestVersionLocations().getLocationListCount(),
+ locationCount);
Review comment:
Thanks!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] adoroszlai commented on pull request #2989: HDDS-6109 Preserve the underlying exception raised in client lib.
Posted by GitBox <gi...@apache.org>.
adoroszlai commented on pull request #2989:
URL: https://github.com/apache/ozone/pull/2989#issuecomment-1012979196
@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:
https://github.com/apache/ozone/blob/3eb7235ca879498c2d8ebcd5fd228d6e4cb16891/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestContainerStateMachineFailures.java#L174-L182
`@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
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] kerneltime edited a comment on pull request #2989: HDDS-6109 Preserve the underlying exception raised in client lib.
Posted by GitBox <gi...@apache.org>.
kerneltime edited a comment on pull request #2989:
URL: https://github.com/apache/ozone/pull/2989#issuecomment-1016075709
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] kerneltime commented on pull request #2989: HDDS-6109 Preserve the underlying exception raised in client lib.
Posted by GitBox <gi...@apache.org>.
kerneltime commented on pull request #2989:
URL: https://github.com/apache/ozone/pull/2989#issuecomment-1012616383
cc @hanishakoneru
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] bharatviswa504 commented on a change in pull request #2989: HDDS-6109 Preserve the underlying exception raised in client lib.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #2989:
URL: https://github.com/apache/ozone/pull/2989#discussion_r784453176
##########
File path: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestContainerStateMachineFailures.java
##########
@@ -245,37 +250,37 @@ public void testContainerStateMachineCloseOnMissingPipeline()
for (HddsDatanodeService dn : datanodeSet) {
LambdaTestUtils.await(20000, 1000,
() -> (dn.getDatanodeStateMachine()
- .getContainer().getContainerSet()
- .getContainer(containerID)
- .getContainerState().equals(QUASI_CLOSED)));
+ .getContainer().getContainerSet()
+ .getContainer(containerID)
+ .getContainerState().equals(QUASI_CLOSED)));
}
key.close();
}
@Test
public void testContainerStateMachineFailures() throws Exception {
OzoneOutputStream key =
- objectStore.getVolume(volumeName).getBucket(bucketName)
- .createKey("ratis", 1024, ReplicationType.RATIS,
- ReplicationFactor.ONE, new HashMap<>());
+ objectStore.getVolume(volumeName).getBucket(bucketName)
+ .createKey("ratis", 1024, ReplicationType.RATIS,
+ ReplicationFactor.ONE, new HashMap<>());
Review comment:
There are lot of space changes, is it intentional??
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] kerneltime edited a comment on pull request #2989: HDDS-6109 Preserve the underlying exception raised in client lib.
Posted by GitBox <gi...@apache.org>.
kerneltime edited a comment on pull request #2989:
URL: https://github.com/apache/ozone/pull/2989#issuecomment-1012616203
Ref: https://github.com/apache/ozone/commit/565972c162819dc4f57d17e4b47f2f47e3bd9c55 https://github.com/apache/ozone/pull/2795
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] kerneltime commented on pull request #2989: HDDS-6109 Preserve the underlying exception raised in client lib.
Posted by GitBox <gi...@apache.org>.
kerneltime commented on pull request #2989:
URL: https://github.com/apache/ozone/pull/2989#issuecomment-1012616203
Ref: https://github.com/apache/ozone/commit/565972c162819dc4f57d17e4b47f2f47e3bd9c55
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] kerneltime commented on a change in pull request #2989: HDDS-6109 Preserve the underlying exception raised in client lib.
Posted by GitBox <gi...@apache.org>.
kerneltime commented on a change in pull request #2989:
URL: https://github.com/apache/ozone/pull/2989#discussion_r786409843
##########
File path: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestContainerStateMachineFailures.java
##########
@@ -677,4 +682,133 @@ public void testWriteStateMachineDataIdempotencyWithClosedContainer()
r2.run();
}
+
+ @Test
+ public void testContainerStateMachineSingleFailureRetry()
+ throws Exception {
+ OzoneOutputStream key =
+ objectStore.getVolume(volumeName).getBucket(bucketName)
+ .createKey("ratis1", 1024, ReplicationType.RATIS,
+ ReplicationFactor.THREE, new HashMap<>());
+
+ key.write("ratis".getBytes(UTF_8));
+ key.flush();
+ key.write("ratis".getBytes(UTF_8));
+ key.write("ratis".getBytes(UTF_8));
+
+ KeyOutputStream groupOutputStream = (KeyOutputStream) key.
+ getOutputStream();
+ List<OmKeyLocationInfo> locationInfoList =
+ groupOutputStream.getLocationInfoList();
+ Assert.assertEquals(1, locationInfoList.size());
+
+ OmKeyLocationInfo omKeyLocationInfo = locationInfoList.get(0);
+
+ Set<HddsDatanodeService> datanodeSet =
+ TestHelper.getDatanodeServices(cluster,
+ omKeyLocationInfo.getPipeline());
+
+ UUID leader = omKeyLocationInfo.getPipeline().getLeaderId();
+ for (HddsDatanodeService dn : datanodeSet) {
+ UUID dnUuid = dn.getDatanodeDetails().getUuid();
+ if (!dnUuid.equals(leader)) {
+ ContainerData containerData =
+ dn.getDatanodeStateMachine()
+ .getContainer().getContainerSet()
+ .getContainer(omKeyLocationInfo.getContainerID())
+ .getContainerData();
+ Assert.assertTrue(containerData instanceof KeyValueContainerData);
+ KeyValueContainerData keyValueContainerData =
+ (KeyValueContainerData) containerData;
+ FileUtil.fullyDelete(new File(keyValueContainerData.getChunksPath()));
+ break;
+ }
+ }
+ try {
+ key.close();
+ } catch (Exception ioe) {
+ // Should not fail..
+ Assert.fail("Exception " + ioe.getMessage());
+ }
+ validateData("ratis1", 2, "ratisratisratis");
+ }
+
+ @Test
+ public void testContainerStateMachineDualFailureRetry()
Review comment:
Will do.
The expected is 2 in this specific targeted case. It is possible to have more than 2 in a real production failure sequence but in a controlled test environment it would be 2.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] kerneltime edited a comment on pull request #2989: HDDS-6109 Preserve the underlying exception raised in client lib.
Posted by GitBox <gi...@apache.org>.
kerneltime edited a comment on pull request #2989:
URL: https://github.com/apache/ozone/pull/2989#issuecomment-1015065962
> @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:
>
>
>
> https://github.com/apache/ozone/blob/3eb7235ca879498c2d8ebcd5fd228d6e4cb16891/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestContainerStateMachineFailures.java#L174-L182
>
>
>
> `@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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] kerneltime commented on pull request #2989: HDDS-6109 Preserve the underlying exception raised in client lib.
Posted by GitBox <gi...@apache.org>.
kerneltime commented on pull request #2989:
URL: https://github.com/apache/ozone/pull/2989#issuecomment-1015065962
> @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:
>
>
>
> https://github.com/apache/ozone/blob/3eb7235ca879498c2d8ebcd5fd228d6e4cb16891/hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestContainerStateMachineFailures.java#L174-L182
>
>
>
> `@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. We would need more than 3. Let fix the annotation and look into performance. Without the restart, the failures induced can lead to other tests being flaky.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] kerneltime commented on pull request #2989: HDDS-6109 Preserve the underlying exception raised in client lib.
Posted by GitBox <gi...@apache.org>.
kerneltime commented on pull request #2989:
URL: https://github.com/apache/ozone/pull/2989#issuecomment-1016075709
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. This should result in no increase in overall test time and 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org
[GitHub] [ozone] bharatviswa504 commented on a change in pull request #2989: HDDS-6109 Preserve the underlying exception raised in client lib.
Posted by GitBox <gi...@apache.org>.
bharatviswa504 commented on a change in pull request #2989:
URL: https://github.com/apache/ozone/pull/2989#discussion_r784453176
##########
File path: hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/client/rpc/TestContainerStateMachineFailures.java
##########
@@ -245,37 +250,37 @@ public void testContainerStateMachineCloseOnMissingPipeline()
for (HddsDatanodeService dn : datanodeSet) {
LambdaTestUtils.await(20000, 1000,
() -> (dn.getDatanodeStateMachine()
- .getContainer().getContainerSet()
- .getContainer(containerID)
- .getContainerState().equals(QUASI_CLOSED)));
+ .getContainer().getContainerSet()
+ .getContainer(containerID)
+ .getContainerState().equals(QUASI_CLOSED)));
}
key.close();
}
@Test
public void testContainerStateMachineFailures() throws Exception {
OzoneOutputStream key =
- objectStore.getVolume(volumeName).getBucket(bucketName)
- .createKey("ratis", 1024, ReplicationType.RATIS,
- ReplicationFactor.ONE, new HashMap<>());
+ objectStore.getVolume(volumeName).getBucket(bucketName)
+ .createKey("ratis", 1024, ReplicationType.RATIS,
+ ReplicationFactor.ONE, new HashMap<>());
Review comment:
There are lot of space changes, is it intentional?? If it is not, can you revert those.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@ozone.apache.org
For additional commands, e-mail: issues-help@ozone.apache.org