-
Notifications
You must be signed in to change notification settings - Fork 514
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
HDDS-10370. Recon - Handle the pre-existing missing empty containers in clusters. #6255
Conversation
@sumitagrawl @dombizita @fapifta , pls review. |
Working on fixing TestContainerHealthTask test case failure. |
This is fixed. |
@@ -113,6 +118,10 @@ public Cursor<UnhealthyContainersRecord> getAllUnhealthyRecordsCursor() { | |||
} | |||
|
|||
public void insertUnhealthyContainerRecords(List<UnhealthyContainers> recs) { | |||
recs.forEach(rec -> { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we wrap this in a "if debug enabled" statement? Normally I am against doing that for a single debug statement, as it is handled inside the logger, but in this case we will have to iterate all the records and it is pssible there are a lot of them, so it might be worth wrapping the entire forEach so avoid that expense if it is not needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok, good suggestion @sodonnel . I have handled it.
@@ -110,38 +112,86 @@ public void testRun() throws Exception { | |||
when(scmClientMock.getContainerWithPipeline(c.getContainerID())) | |||
.thenReturn(new ContainerWithPipeline(c, null)); | |||
} | |||
|
|||
ReplicatedReplicationConfig replicationConfig = new ReplicatedReplicationConfig() { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need all this? Can we not just do:
ReplicatedReplicationConfig replicationConfig = RatisReplicationConfig.getInstance(THREE);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we need all this? Can we not just do:
ReplicatedReplicationConfig replicationConfig = RatisReplicationConfig.getInstance(THREE);
Yes, its not needed. I added earlier due to a error. Now simplified. Thanks.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…in clusters. (apache#6255) (cherry picked from commit e0bf7b4) Change-Id: I72150151e39f2bfc29d9f39673782c1afd5353ad
What changes were proposed in this pull request?
This PR addresses the corner case when a Recon has earlier identified some MISSING (UNHEALTHY) containers, but they all were EMPTY (No keys mapped), so Recon should filter out such existing MISSING (UNHEALTHY) containers.
In a running cluster, Recon may have identified some MISSING (UNHEALTHY) containers before the HDDS-9695 was applied. After the fix is applied, in a running cluster, existing MISSING (UNHEALTHY) containers which were actually EMPTY were still shown as MISSING, so this PR change is to filter out such existing MISSING (UNHEALTHY) containers which were actually EMPTY.
What is the link to the Apache JIRA
https://issues.apache.org/jira/browse/HDDS-10370
How was this patch tested?
Existing Junit integration test
org.apache.hadoop.ozone.recon.TestReconTasks#testEmptyMissingContainerDownNode
was updated and tested.