Skip to content

HDDS-9524. Clean up wait for leader OM in integration tests#6092

Merged
adoroszlai merged 3 commits intoapache:masterfrom
david1859168:HDDS-9524
Jan 26, 2024
Merged

HDDS-9524. Clean up wait for leader OM in integration tests#6092
adoroszlai merged 3 commits intoapache:masterfrom
david1859168:HDDS-9524

Conversation

@david1859168
Copy link
Contributor

What changes were proposed in this pull request?

MiniOzoneHAClusterImpl:

getOMLeader() returns current OM leader.
getOMLeader(boolean) optionally waits until OM leader is elected
There are some tests that implement the same "wait for leader OM" logic using the non-waiting getOMLeader().

hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestAddRemoveOzoneManager.java
209:    GenericTestUtils.waitFor(() -> cluster.getOMLeader() != null, 500, 30000);
437:    GenericTestUtils.waitFor(() -> cluster.getOMLeader() != null, 500, 30000);

hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOzoneManagerHA.java
466:        getCluster().getOMLeader() != null, 500, timeout);

hadoop-ozone/integration-test/src/test/java/org/apache/hadoop/ozone/om/TestOzoneManagerHASnapshot.java
126:    await(120_000, 100, () -> cluster.getOMLeader() != null);
253:    await(180_000, 100, () -> cluster.getOMLeader() != null);

Goals of this task:
rename getOMLeader(boolean) to waitForLeaderOM() (the method is only called with true)
make waitForLeaderOM() public
replace duplicated wait logic in tests with a call to waitForLeaderOM()

What is the link to the Apache JIRA

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

How was this patch tested?

Made changes and ran unit test.

Copy link
Contributor

@adoroszlai adoroszlai left a comment

Choose a reason for hiding this comment

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

Thanks @david1859168 for the patch. Findbugs reports an unused variable:

M D DLS: Dead store to timeout in org.apache.hadoop.ozone.om.TestOzoneManagerHA.waitForLeaderToBeReady()  At TestOzoneManagerHA.java:[line 463]

https://github.com/david1859168/ozone/actions/runs/7652102229/job/20851218541#step:7:11

Copy link
Contributor

@adoroszlai adoroszlai left a comment

Choose a reason for hiding this comment

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

Thanks @david1859168 for updating the patch, LGTM.

Copy link
Member

@aswinshakil aswinshakil left a comment

Choose a reason for hiding this comment

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

Nice find. Thanks for the patch @david1859168 and Thanks for the review @adoroszlai

@adoroszlai adoroszlai merged commit 305460d into apache:master Jan 26, 2024
@adoroszlai
Copy link
Contributor

Thanks @david1859168 for the patch, @aswinshakil for the review.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants