HDDS-9819. Recon - Potential memory overflow in Container Health Task. #5841

devmadhuu · 2023-12-20T12:33:46Z

What changes were proposed in this pull request?

This PR addresses the issue of potential. memory overflow in case there are millions of containers because ContainerHealthTask tries to load all containers available in memory using containerManager.getContainers(), this API loads maximum available containers. So to avoid any memory overflow, this PR changes the API to use paginated based API.

What is the link to the Apache JIRA

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

How was this patch tested?

This patch was tested using existing junit test cases in order not to break anything.

devmadhuu · 2023-12-20T12:34:01Z

@sumitagrawl @adoroszlai pls review.

adoroszlai

Thanks @devmadhuu for the patch.

adoroszlai · 2023-12-20T16:05:30Z

hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/fsck/ContainerHealthTask.java

                                    Map<String, Long>>
-                                      unhealthyContainerStateStatsMap) {
+                                    unhealthyContainerStateStatsMap) {


nit: Please avoid unnecessary space changes.

adoroszlai · 2023-12-20T16:05:39Z

hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/fsck/ContainerHealthTask.java

      if (container.isUnderReplicated()
          && !recordForStateExists.contains(
-              UnHealthyContainerStates.UNDER_REPLICATED.toString())) {
+          UnHealthyContainerStates.UNDER_REPLICATED.toString())) {


nit: Please avoid unnecessary space changes.

adoroszlai · 2023-12-20T16:08:03Z

hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/fsck/ContainerHealthTask.java

+
+  private void checkAndProcessContainers(
+      Map<UnHealthyContainerStates, Map<String, Long>>
+          unhealthyContainerStateStatsMap, long start, long currentTime) {


start should not be a parameter, it should be set at the start of each iteration.

Ok, I made the changes. Thanks.

adoroszlai · 2023-12-21T16:27:58Z

@devmadhuu please check if test failure in TestReconTasks is related:

https://github.com/devmadhuu/ozone/actions/runs/7283607925/job/19848007957#step:6:167

devmadhuu · 2024-01-05T06:08:39Z

@devmadhuu please check if test failure in TestReconTasks is related:

https://github.com/devmadhuu/ozone/actions/runs/7283607925/job/19848007957#step:6:167

Thanks @adoroszlai for review. I have resolved the test case failure.

adoroszlai · 2024-01-05T09:09:11Z

Thanks @devmadhuu for updating the patch.

@ArafatKhan2198 please review

ArafatKhan2198

Thanks for the work on this @devmadhuu I have left a few comments.

ArafatKhan2198 · 2024-01-08T08:09:06Z

hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/fsck/ContainerHealthTask.java

+          unhealthyContainerStateStatsMap, long currentTime) {
+    ContainerID startID = ContainerID.valueOf(1);
+    List<ContainerInfo> containers = containerManager.getContainers(startID,
+        Integer.parseInt(DEFAULT_FETCH_COUNT));


Could we replace the usage of Integer.parseInt(DEFAULT_FETCH_COUNT) with a named constant as DEFAULT_FETCH_COUNT is a constant.

private static final int FETCH_COUNT = Integer.parseInt(DEFAULT_FETCH_COUNT);

ArafatKhan2198 · 2024-01-08T08:11:55Z

hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/fsck/ContainerHealthTask.java

+        containers = containerManager.getContainers(startID,
+            Integer.parseInt(DEFAULT_FETCH_COUNT));
+      }
+      containers.clear();


The containers.clear(); statement inside the if block is meant to clear the list before fetching the next batch. However, the last containers.clear(); statement just outside the if block seems redundant since the loop condition (while (!containers.isEmpty())) ensures that it's only executed when containers is empty.?

@ArafatKhan2198 I couldn't understand this comment, as I don't see containers.clear() outside the loop. Can you pls clarify ?

Apologies for the earlier mistake in the comment. I have now corrected it. Please take a look.

As discussed, we have a case where last iteration in batch will not execute the if condition , so need to clear -off any in memory.

ArafatKhan2198 · 2024-01-10T11:30:37Z

Thanks for the changes @devmadhuu LGTM!

sumitagrawl

@devmadhuu Thanks for working over this, have few comments

hadoop-ozone/recon/src/main/java/org/apache/hadoop/ozone/recon/fsck/ContainerHealthTask.java

sumitagrawl

LGTM +1

adoroszlai

Thanks @devmadhuu for updating the patch. Please be careful when merging changes from master (sorry for the conflict).

adoroszlai · 2024-01-14T14:37:31Z

...op-ozone/recon/src/test/java/org/apache/hadoop/ozone/recon/fsck/TestContainerHealthTask.java


    List<UnhealthyContainers> all = unHealthyContainersTableHandle.findAll();
-    assertThat(all).isEmpty();
+    assertTrue(all.isEmpty());


Please keep assertThat, added by HDDS-10034.

Suggested change

assertTrue(all.isEmpty());

assertThat(all).isEmpty();

adoroszlai · 2024-01-14T14:38:09Z

...op-ozone/recon/src/test/java/org/apache/hadoop/ozone/recon/fsck/TestContainerHealthTask.java

+    assertTrue(taskStatus.getLastUpdatedTimestamp() >
+        currentTime);


Please keep assertThat, added by HDDS-10034.

Suggested change

assertTrue(taskStatus.getLastUpdatedTimestamp() >

currentTime);

assertThat(taskStatus.getLastUpdatedTimestamp()).isGreaterThan(currentTime);

adoroszlai · 2024-01-14T14:38:50Z

...op-ozone/recon/src/test/java/org/apache/hadoop/ozone/recon/fsck/TestContainerHealthTask.java

+    assertTrue(taskStatus.getLastUpdatedTimestamp() >
+        currentTime);


Please keep assertThat, added by HDDS-10034.

Suggested change

assertTrue(taskStatus.getLastUpdatedTimestamp() >

currentTime);

assertThat(taskStatus.getLastUpdatedTimestamp()).isGreaterThan(currentTime);

adoroszlai · 2024-01-14T14:39:04Z

...op-ozone/recon/src/test/java/org/apache/hadoop/ozone/recon/fsck/TestContainerHealthTask.java


    List<UnhealthyContainers> all = unHealthyContainersTableHandle.findAll();
-    assertThat(all).isEmpty();
+    assertTrue(all.isEmpty());


Please keep assertThat, added by HDDS-10034.

Suggested change

assertTrue(all.isEmpty());

assertThat(all).isEmpty();

adoroszlai · 2024-01-14T14:39:50Z

...op-ozone/recon/src/test/java/org/apache/hadoop/ozone/recon/fsck/TestContainerHealthTask.java

-import static org.assertj.core.api.Assertions.assertThat;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
+import static org.junit.jupiter.api.Assertions.assertTrue;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.anyInt;


Please keep assertThat instead of restoring assertTrue.

adoroszlai · 2024-01-14T16:14:28Z

Thanks @devmadhuu for the patch, @ArafatKhan2198, @sumitagrawl for the review.

apache#5841) (cherry picked from commit 1398f58)

HDDS-9819.Recon - Potential memory overflow in Container Health Task.

6f411d1

adoroszlai reviewed Dec 20, 2023

View reviewed changes

Fixed review comments.

c77e3d5

adoroszlai added the recon label Dec 21, 2023

deveshsingh added 2 commits January 2, 2024 14:48

HDDS-9819.Fixed Test case failures in TestReconTasks.

7a7e68f

HDDS-9819.Fixed Test case failures in TestReconTasks.

88b671f

adoroszlai changed the title ~~HDDS-9819.Recon - Potential memory overflow in Container Health Task.~~ HDDS-9819. Recon - Potential memory overflow in Container Health Task. Jan 3, 2024

Merge remote-tracking branch 'origin/master' into HDDS-9819

1a2c4ce

adoroszlai requested a review from dombizita January 5, 2024 09:09

ArafatKhan2198 reviewed Jan 8, 2024

View reviewed changes

deveshsingh added 3 commits January 10, 2024 11:25

Merge remote-tracking branch 'origin/master' into HDDS-9819

720d24b

HDDS-9819.Fixed review comments.

81f271c

HDDS-9819.Fixed review comments.

903805a

ArafatKhan2198 approved these changes Jan 10, 2024

View reviewed changes

sumitagrawl reviewed Jan 10, 2024

View reviewed changes

HDDS-9819.Fixed review comments.

8612090

sumitagrawl approved these changes Jan 14, 2024

View reviewed changes

adoroszlai requested changes Jan 14, 2024

View reviewed changes

adoroszlai added 2 commits January 14, 2024 15:43

Merge remote-tracking branch 'origin/master' into HDDS-9819

0abeec2

restore changes from HDDS-10034 (lost in merge)

1fb54db

adoroszlai approved these changes Jan 14, 2024

View reviewed changes

adoroszlai merged commit 1398f58 into apache:master Jan 14, 2024

xichen01 pushed a commit to xichen01/ozone that referenced this pull request Sep 18, 2024

HDDS-9819. Recon - Potential memory overflow in Container Health Task. (

45b71c8

apache#5841) (cherry picked from commit 1398f58)

xichen01 mentioned this pull request Sep 19, 2024

[DO NOT MERGE] Backport some fixes and compatibility commits from master to ozone-1.4 #7218

Merged

		assertTrue(taskStatus.getLastUpdatedTimestamp() >
		currentTime);

	assertTrue(taskStatus.getLastUpdatedTimestamp() >
	currentTime);
	assertThat(taskStatus.getLastUpdatedTimestamp()).isGreaterThan(currentTime);

HDDS-9819. Recon - Potential memory overflow in Container Health Task. #5841

HDDS-9819. Recon - Potential memory overflow in Container Health Task. #5841

Uh oh!

Conversation

devmadhuu commented Dec 20, 2023

What changes were proposed in this pull request?

What is the link to the Apache JIRA

How was this patch tested?

Uh oh!

devmadhuu commented Dec 20, 2023

Uh oh!

adoroszlai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adoroszlai commented Dec 21, 2023

Uh oh!

devmadhuu commented Jan 5, 2024

Uh oh!

adoroszlai commented Jan 5, 2024

Uh oh!

ArafatKhan2198 left a comment

Choose a reason for hiding this comment

Uh oh!

ArafatKhan2198 Jan 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ArafatKhan2198 Jan 8, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ArafatKhan2198 commented Jan 10, 2024

Uh oh!

sumitagrawl left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sumitagrawl left a comment

Choose a reason for hiding this comment

Uh oh!

adoroszlai left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

adoroszlai commented Jan 14, 2024

Uh oh!

Reviewers

Assignees

ArafatKhan2198 Jan 8, 2024 •

edited

Loading

ArafatKhan2198 Jan 8, 2024 •

edited

Loading