Skip to content

Conversation

@wzhallright
Copy link
Contributor

What changes were proposed in this pull request?

Simplify tests using assertThrows in hadoop-hdds

What is the link to the Apache JIRA

HDDS-10208

CI

https://github.com/wzhallright/ozone/actions/runs/7737861963

Copy link
Contributor

@fapifta fapifta left a comment

Choose a reason for hiding this comment

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

Thank you for the path @wzhallright the changes look good except that one change I left an inline comment on, can you please elaborate on that?


@Test
public void testAddAndGetContainer() throws IOException, TimeoutException {
long containerID = 0;
Copy link
Contributor

Choose a reason for hiding this comment

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

May I ask what is the rationale behind changing this long to AtomicLong? The rest of the patch is really just changing exception testing to assertThrows, is it by chance a leftover from some other work, or intentional?

Copy link
Contributor

Choose a reason for hiding this comment

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

I assume it is changed to be able to use in this lambda expression:

IOException e =
assertThrows(IOException.class,
() -> stateManager.addContainerToPipeline(finalPipeline.getId(),
ContainerID.valueOf(containerID.incrementAndGet())));

I think it's fine. If we want to minimize the diff, we could keep long containerID and change the code above to something like this:

    ContainerID cid = ContainerID.valueOf(++containerID);
    IOException e = assertThrows(IOException.class,
        () -> stateManager.addContainerToPipeline(finalPipeline.getId(), cid));

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for your review! the IntelliJ prompts me variable used in lambda expression should be final or effectively final, so i change long to AtomicLong. Perhaps there is a more elegant solution to this problem?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@adoroszlai That's a good idea. I will modify it. Thanks

Copy link
Contributor

Choose a reason for hiding this comment

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

thank you for the explanation, I did not catch that point. my bad.

@adoroszlai adoroszlai added the test label Feb 1, 2024
@adoroszlai adoroszlai merged commit f8d54b3 into apache:master Feb 1, 2024
@adoroszlai
Copy link
Contributor

Thanks @wzhallright for the patch, @fapifta for the review.

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants